-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for secret length in bytes and additional encodings #29
Add support for secret length in bytes and additional encodings #29
Conversation
@@ -14,6 +14,7 @@ const ( | |||
AnnotationSecretType = "secret-generator.v1.mittwald.de/type" | |||
AnnotationSecretLength = "secret-generator.v1.mittwald.de/length" | |||
AnnotationBasicAuthUsername = "secret-generator.v1.mittwald.de/basic-auth-username" | |||
AnnotationSecretEncoding = "secret-generator.v1.mittwald.de/encoding" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't forget to also adjust the README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I have a qualm with one minor detail, though - see comment inline. I didn't elaborate on this detail in my original spec from #27 because I was worried it was already too wordy as it was 😅
|
||
The length of the generated secret can be specified by the `secret-generator.v1.mittwald.de/length` annotation. | ||
By default, this length refers to the length of the generated string, and not the length of the byte sequence encoded by it. | ||
The suffix `B` or `b` can be used to indicate that the provided value should refer to the encoded byte sequence instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the one quibble I was hoping I wouldn't have to bring up by submitting the PR myself: the lower-case suffix b
should not be recognized as a suffix for byte count, as b
is the IEEE suffix for bits, not bytes, and should technically divide the value by 8 - a value of 64b
is equivalent to 8B
. (This is more commonly recognized in forms like "Mbps" for "megabits per second", where the number of megabytes that will be downloaded in a second must be calculating by dividing by eight; more common to this application's use case, the bit-count is often used as the unit measuring the length of key material, ie. 2048b
for a 2048-bit key, rather than expressing it as "256 bytes").
If anything, the supported suffixes should be b
for bits and o
for octets, the case-insentive and unambiguous suffix for a byte (technically a different unit, though this is a distinction that dates back to the dinosaur days, back when there were systems that actually had non-eight-bit bytes).
At the same time, I recognize that users using b
as a suffix are more likely to intend the b
to stand for "bytes" and not bits - though either intended interpretation is plausible - which is why I believe that this letter's lower case ought to result in an error (ie. no generated value and a logged message as to why), rather than interpreted under the assumption of one way or the other. (If the user wants to specify a number of bits not divisible by eight, they ought to express it in terms of characters expressed in the intended encoding.)
In theory, which way "b" should be interpreted (ie. whether it should be byte-presumptive or IEEE-compliant) could be configured as part of the operator's runtime configuration, but, in my experience, providing that kind of "flexibility" can be understood akin to introducing a microscopically forked version of your project: it just introduces more incompatibilities that can emerge between live deployments, and there's no reason to introduce that kind of incompatibility unless there's no other option.
Closes #27