Timeline for Password maker in C#
Current License: CC BY-SA 4.0
36 events
| when toggle format | what | by | license | comment | |
|---|---|---|---|---|---|
| Jul 13, 2019 at 19:14 | history | edited | Kittoes0124 | CC BY-SA 4.0 | deleted 44 characters in body |
| Jul 13, 2019 at 18:07 | comment | added | Kittoes0124 | Let us continue this discussion in chat. | |
| Jul 13, 2019 at 18:05 | history | edited | Kittoes0124 | CC BY-SA 4.0 | added 87 characters in body |
| Jul 13, 2019 at 18:05 | comment | added | jpmc26 | I'm talking about the kind of mistake the OP made. They created multiple instances of an RNG, which actually reduces the randomness. It may not be clear to some developers that the instance needs to be shared. "one huge reason for the interface is to send PasswordGenerator predictable inputs." This doesn't make sense to me because it's required that PasswordGenerator's output be random. As a result, sending it predictable inputs breaks its interface. I don't consider such a unit test to be meaningful. | |
| Jul 13, 2019 at 18:01 | comment | added | Kittoes0124 | @jpmc26 "That will silently prevent the very randomness we seek to enforce." What in the world are you talking about?! An RNG class that allows multiple instances isn't inherently broken in any way; some implementations might be broken but that doesn't mean we're forced to code around them. Requiring the ISecureRandom interface is a rather strong contract since no wild class is going to inherently have it; and yes, I do want to fake out the thing that uses the RNG... one huge reason for the interface is to send PasswordGenerator predictable inputs. | |
| Jul 13, 2019 at 17:39 | comment | added | jpmc26 | @Kittoes0124 "...if they want to pass in a different RNG to each instance then so be it!" No. That will silently prevent the very randomness we seek to enforce. Code should be written to reduce risks. As for unit testing, no. Your unit tests should not try to replace an RNG directly. Ever. You should not be writing tests that assert on specific outputs when randomness is a core requirement of the method. Maybe you might want to fake out the thing that uses the RNG, but not the RNG itself. It's typically better to convert randomly generated items into a input, though, and avoid the coupling. | |
| Jul 13, 2019 at 17:15 | comment | added | Kittoes0124 | @jpmc26 What you're saying about "many instances" is just nonsense though, it isn't up to us to decide how another developer uses their own tools; if they want to pass in a different RNG to each instance then so be it! The decoupling might be generally useless in most production scenarios but it ends up being immensely valuable in other contexts, such as unit testing. I understand where you're coming from regarding complication but it is hard to take you seriously when you're essentially complaining that arbitrary magic constants were converted into proper variables. | |
| Jul 13, 2019 at 16:56 | comment | added | jpmc26 | @Kittoes0124 Regarding "amplifying," that is often how we get ourselves into trouble overcomplicating our problems. I would rather discourage this tendency for people asking for review than jump into it ourselves. | |
| Jul 13, 2019 at 16:52 | comment | added | jpmc26 | @Kittoes0124 The interface is a bad idea. Injecting RNGs leads to the mistake of creating many instances. PasswordGenerator would not contain any more direct knowledge of the implementation RNG either way. And in fact, you could still inject an instance of RNGCryptoServiceProvider just as easily, if you absolutely insisted. There's no useful decoupling accomplished by it. | |
| Jul 13, 2019 at 16:40 | comment | added | Kittoes0124 | @jpmc26 While simpler, the static extension method wouldn't have been able to implement the ISecureRandom interface; a key feature that keeps the PasswordGenerator completely ignorant of how the numbers are generated (notice that the parameterless constructor now falls back to using a statically defined readonly rng). Varying the minimum characters might not be a requirement but it makes the code more generally useful. The OP actually has a decent approach and it seemed like a good idea to "amplify" it by making different variables available to a consumer. | |
| Jul 13, 2019 at 16:39 | history | edited | Kittoes0124 | CC BY-SA 4.0 | added 72 characters in body |
| Jul 13, 2019 at 16:24 | history | edited | Kittoes0124 | CC BY-SA 4.0 | deleted 7 characters in body |
| Jul 13, 2019 at 16:05 | comment | added | jpmc26 | Varying the minimum number of each type of character is also not a requirement and is an additional complication that is not needed here. | |
| Jul 13, 2019 at 15:47 | comment | added | jpmc26 | An extension method would be far simpler than using a completely separate class for generating integers, which is only required for the shuffle anyway. I also recommend against injecting instances of random number generators. A very common mistake in using RNGs is creating multiple instances rather than reusing the same one. Having a single, static final instance will discourage this mistake and give you a place to leave a comment reminding everyone to not to new up their own instance. | |
| Jul 13, 2019 at 13:54 | comment | added | Vilx- | The default Random class uses the system time as the seed. More specifically, the Environment.TickCount. This makes it REALLY EASY to guess the seed. There are so few options, you can just check them all! So, yes, in a serious implementation, using the RNGCryptoServiceProvider is a must. For homework, of course, the default Random class will be just fine. | |
| Jul 12, 2019 at 23:40 | comment | added | Ghedipunk | Very, very firm uptick for using RNGCryptoServiceProvider. While yes, it's very unlikely that an attacker will get the seed, it's always best to use cryptographically secure system when working with any part of authentication, especially CSPRNGs over DPRNGs. | |
| Jul 12, 2019 at 21:46 | history | edited | Kittoes0124 | CC BY-SA 4.0 | added interface to make it easier for others to replace my possibly shitty SecureRandom implementation |
| Jul 12, 2019 at 21:35 | comment | added | Kittoes0124 | @RolandIllig Sadly, there really isn't anything in the standard lib; I'd have used it. One concedes that the code is bloated but figured that having parameters is more "maintainable" in the long run; for example, one might be dealing with many clients that all have different configurations (or, heaven forbid, a client that changes their requirements over time). | |
| Jul 12, 2019 at 21:27 | comment | added | Roland Illig | This code looks really bloated, compared to the original code. Generating a password using a given simple and fixed scheme should take no more than 30 lines of code. | |
| Jul 12, 2019 at 21:23 | comment | added | Roland Illig | Is there really no predefined SecureRandom class in .NET? It sounds really strange that everyone would have to write this class by themselves. | |
| Jul 12, 2019 at 21:15 | history | edited | Kittoes0124 | CC BY-SA 4.0 | added 6 characters in body |
| Jul 12, 2019 at 21:09 | history | edited | Kittoes0124 | CC BY-SA 4.0 | added 17 characters in body |
| Jul 12, 2019 at 21:02 | history | edited | Kittoes0124 | CC BY-SA 4.0 | updated code to use cryptographic random byte generator |
| Jul 12, 2019 at 20:03 | history | edited | Kittoes0124 | CC BY-SA 4.0 | deleted 5 characters in body |
| Jul 12, 2019 at 19:51 | history | edited | Kittoes0124 | CC BY-SA 4.0 | deleted 30 characters in body |
| Jul 12, 2019 at 18:52 | history | edited | Kittoes0124 | CC BY-SA 4.0 | added 254 characters in body |
| Jul 12, 2019 at 18:49 | history | edited | Kittoes0124 | CC BY-SA 4.0 | added 254 characters in body |
| Jul 12, 2019 at 18:41 | history | edited | Kittoes0124 | CC BY-SA 4.0 | added 50 characters in body |
| Jul 12, 2019 at 18:28 | history | edited | Kittoes0124 | CC BY-SA 4.0 | added 1422 characters in body |
| Jul 12, 2019 at 17:52 | history | edited | Kittoes0124 | CC BY-SA 4.0 | added 10 characters in body |
| Jul 12, 2019 at 17:30 | history | edited | Kittoes0124 | CC BY-SA 4.0 | added 14 characters in body |
| Jul 12, 2019 at 17:11 | history | edited | Kittoes0124 | CC BY-SA 4.0 | added 87 characters in body |
| Jul 12, 2019 at 17:07 | history | edited | Kittoes0124 | CC BY-SA 4.0 | added 1544 characters in body |
| Jul 12, 2019 at 16:52 | history | edited | Kittoes0124 | CC BY-SA 4.0 | added 1544 characters in body |
| Jul 12, 2019 at 16:45 | review | Low quality posts | |||
| Jul 12, 2019 at 17:00 | |||||
| Jul 12, 2019 at 16:27 | history | answered | Kittoes0124 | CC BY-SA 4.0 |