Skip to content
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

config: Add base64sha256() function #4899

Merged
merged 2 commits into from
Jan 30, 2016

Conversation

radeksimko
Copy link
Member

This will help finishing #3825 and solve the problem I described in #3825 (comment)

I was thinking about the name, considering base64encodedsha256(), but it seemed a bit too long and unnecessary to keep "encoded" there. It is very unlikely we'll ever need something like base64decodedsha256(). That is why I chosen a simple name base64sha256.

@apparentlymart
Copy link
Contributor

This seems fine to me.

I have a third formulation to consider, though: could we have a variant of the base64 function that expects its parameter to be a set of octets expressed as a hexadecimal string? Something like `base64fromhex(sha256(foo))``, where it would error if its argument is not a string containing an even number of hex digits.

This alternative makes things a bit more orthogonal (could be used with sha1 also, for example) but I expect it'd be kinda hard to explain what base64fromhex does in the docs, as distinguished from the regular base64.

@radeksimko
Copy link
Member Author

I like the idea of detecting hex vs bytes, I'm just wondering what would be the percentage of use-cases where you actually want base64-encode hexadecimal representation.

My personal humble guess is that user is more likely going to hit a problem when encoding hex while actually wanting to encode raw bytes - i.e. I think either the current base64() should be checking for hex (and return error/warning) or we should have base64frombytes(), which would just be the current base64() except erroring out on non-byte output (which may not be easy to detect since everything is a byte after all). 💭

I'm taking

This seems fine to me.

as 👍 and will merge this PR during the weekend, unless someone comes up with a reason why not to then.

@radeksimko
Copy link
Member Author

Ah, hold on...

Maybe I misunderstood. Did you want base64fromhex to actually do the hex decoding to get the raw bytes and then do the base64 encoding of those bytes?

If yes, then I think it will be hard to explain in the docs, as you mentioned and probably even harder to come up with a good name for such function.

@apparentlymart
Copy link
Contributor

I think the difficulty of communicating my idea within this PR is sufficient evidence that it's a bad idea. :)

And yeah, I was meaning 👍 for implementation, ignoring the merits of this particular design. I'm less sure about whether this is the best design for solving this problem, but I think we just have a set of sub-optimal approaches here and no obvious winner so I'm not going to argue. 😀

@@ -80,12 +80,17 @@ The supported built-in functions are:
* `base64encode(string)` - Returns a base64-encoded representation of the
given string.

* `sha1(string)` - Returns a SHA-1 hash representation of the
given string.
* `base64sha256(string)` - Returns a base64-encoded represantation of raw
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"representation"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.

@radeksimko
Copy link
Member Author

I guess it will be also helpful to add support for warnings & deprecation for built-in functions, so that if we come up with a better solution in the future, we can communicate that gracefully to users.

I'd also say that's out of the scope of this PR and the linked Lambda PR, though.

radeksimko added a commit that referenced this pull request Jan 30, 2016
config: Add base64sha256() function
@radeksimko radeksimko merged commit 9db941a into hashicorp:master Jan 30, 2016
@radeksimko radeksimko deleted the f-base64-sha256 branch January 30, 2016 13:06
@ghost
Copy link

ghost commented Apr 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants