Make errors faithful

[XaviFP]

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]

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]

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.