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

Make errors faithful #6

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

XaviFP
Copy link

@XaviFP XaviFP commented Jul 13, 2022

Just leaving this here as a conversation starter.

I might not be aware of all details, but, is there any particular reason to compute new keys even though they must not be used?
Since an error is returned, is there a case where use of randomly generated keys would make sense?

I just stumbled upon this as I tried to build a v4 key pair from bytes coming from a x25519 pem certificate.
I had some invisible character slip into the certificate string due to automatic code formatting, so the byte slice representing the key was off, yet I was obtaining a fully functional key.

@aidantwoods
Copy link
Owner

Hey, thanks for bringing this up!

I just stumbled upon this as I tried to build a v4 key pair from bytes coming from a x25519 pem certificate.
I had some invisible character slip into the certificate string due to automatic code formatting, so the byte slice representing the key was off, yet I was obtaining a fully functional key.

I'm curious how you ended up encountering this? Did you opt to try to check the key value instead of checking the err value? AFAIK even the Go standard library will return a non-zero value paired with an error under some circumstances, and so I think most commonly the convention is the check the err value to determine success.

I might not be aware of all details, but, is there any particular reason to compute new keys even though they must not be used?
Since an error is returned, is there a case where use of randomly generated keys would make sense?

The main motivation here was to avoid returning a predictable key in the event that an error is emitted, but for some reason ignored. i.e. an error in the key shouldn't end up creating a key full of zero bytes (which someone else could also construct, and forge compatible PASETOs).

I could probably be persuaded to return pointers to keys instead of the struct values themselves so that we could return nil instead of a randomly generated key.

However, I'd like to avoid needing to accept pointers in the functions which consume these keys, because it would mean that certain functions which don't need to emit errors at the moment now would need to because they'd need to check if the pointer was to nil. If these functions don't accept pointers, then there is a slight ergonomic adjustment that the user needs to make when using them, as they'd need to manually deref the pointer. This could be acceptable, but would be a bit of a BC break.

@XaviFP
Copy link
Author

XaviFP commented Jul 14, 2022

I'm curious how you ended up encountering this? Did you opt to try to check the key value instead of checking the err value? AFAIK even the Go standard library will return a non-zero value paired with an error under some circumstances, and so I think most commonly the convention is the check the err value to determine success.

Yes, kind of :). I was comparing the key I was getting to the one I was expecting (given that I was passing in the bytes for it). I realized keys didn't match, kind of expected as I was getting a non nil error, but the unexpected part for me was to get a non nil/zero-value fully working key instead.

The main motivation here was to avoid returning a predictable key in the event that an error is emitted, but for some reason ignored. i.e. an error in the key shouldn't end up creating a key full of zero bytes (which someone else could also construct, and forge compatible PASETOs).

I do agree with you in avoiding to return a predictable key, especially when it comes to the zero-value.
And I also agree that as long as one checks the error and acts accordingly there's nothing to fear. So it's not a matter of correctness but rather actively preventing mistakes by misuse.

I believe returning a non-working value is safer altogether, as you point out, a pointer would be a safer default albeit more work should be done around it and the user experience might be slightly decreased.

All in all I see and understand your point :)

Anyway, if you are up for it I don't mind dropping in an exploratory PR and we could take it from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants