Skip to content

Fix error types emitted by good_key.go#4932

Merged
jsha merged 4 commits intomainfrom
goodkey-errors
Jul 6, 2020
Merged

Fix error types emitted by good_key.go#4932
jsha merged 4 commits intomainfrom
goodkey-errors

Conversation

@aarongable
Copy link
Contributor

@aarongable aarongable commented Jul 2, 2020

The KeyPolicy.GoodKey method is used to validate both public keys
used to sign JWK messages, and public keys contained inside CSR
messages.

According to RFC8555 section 6.7, validation failure in the former
case should result in badPublicKey, while validation failure in
the latter case should result in badCSR. In either case, a failure
due to reasons other than the key itself should result in
serverInternal.

However, the GoodKey method returns a variety of different errors
which are not all applicable depending on the context in which it is
called. In addition, the csr.VerifyCSR method passes these errors
through verbatim, resulting in ACME clients receiving confusing and
incorrect error message types.

This change causes the GoodKey method to always return either a
generic error or a KeyError. Calling methods should treat a KeyError
as either a badPublicKey or a badCSR depending on their context,
and may treat a generic error however they choose (though likely as a
serverInternal error).

Fixes #4930

@jsha
Copy link
Contributor

jsha commented Jul 2, 2020

Nice improvement! I don't see anything substantive lacking here - what else are you wanting to get done before taking it out of draft status?

@aarongable
Copy link
Contributor Author

Just waiting on green travis. Looks like I have one more integration test to fix (only shows up in config-next).

@aarongable aarongable marked this pull request as ready for review July 2, 2020 23:27
@aarongable aarongable requested a review from a team as a code owner July 2, 2020 23:27
@jsha jsha merged commit 4a85abf into main Jul 6, 2020
@jsha jsha deleted the goodkey-errors branch July 6, 2020 17:06
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.

sa.StorageAuthority.KeyBlocked timeout returns wrong error

3 participants