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

More specific error for unexpected cert request #4381

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

lrstewart
Copy link
Contributor

Description of changes:

A customer saw a confusing S2N_ERR_BAD_MESSAGE error when their server requested client auth but their client wasn't configured to support client auth. I'm adding a more specific error, since this seems like it would be a fairly common situation.

Call-outs:

I'm not changing any behavior here, but is the current behavior correct? It seems to me like a client shouldn't fail if the server requests client auth, it should just act like it does when S2N_CERT_AUTH_OPTIONAL is chosen and send an empty certificate. Basically, I think clients should default to S2N_CERT_AUTH_OPTIONAL instead of S2N_CERT_AUTH_NONE.

A better error is an improvement, but I'm wondering if the correct fix is a behavior change.

Testing:

Added a new self-talk test for the handshake.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions github-actions bot added the s2n-core team label Jan 26, 2024
@lrstewart lrstewart marked this pull request as ready for review January 26, 2024 19:55
Copy link
Contributor

@goatgoose goatgoose left a comment

Choose a reason for hiding this comment

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

Basically, I think clients should default to S2N_CERT_AUTH_OPTIONAL instead of S2N_CERT_AUTH_NONE.

Yeah, this makes sense to me. Since this is currently inherited by the config, we should just be careful not to enable this by default for servers.

@maddeleine
Copy link
Contributor

Basically, I think clients should default to S2N_CERT_AUTH_OPTIONAL instead of S2N_CERT_AUTH_NONE.

Yeah, this makes sense to me. Since this is currently inherited by the config, we should just be careful not to enable this by default for servers.

Any idea why we had this behavior in the first place? It does seem weird to error on the client-side in this case.

@lrstewart lrstewart enabled auto-merge (squash) January 26, 2024 22:22
@lrstewart
Copy link
Contributor Author

Basically, I think clients should default to S2N_CERT_AUTH_OPTIONAL instead of S2N_CERT_AUTH_NONE.

Yeah, this makes sense to me. Since this is currently inherited by the config, we should just be careful not to enable this by default for servers.

Any idea why we had this behavior in the first place? It does seem weird to error on the client-side in this case.

Nope. We'd need to investigate before making the change. Changing default behavior is always risky-- maybe it was just a mistake, or maybe there was a good reason for it we're just not guessing.

@lrstewart lrstewart merged commit 02c6089 into aws:main Jan 26, 2024
31 checks passed
@lrstewart lrstewart deleted the fix_error branch January 26, 2024 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants