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

Further secure TLS communications #97

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

saroshali-dbx
Copy link
Contributor

Closes #96

Signed-off-by: saroshali-dbx <[email protected]>
Signed-off-by: saroshali-dbx <[email protected]>
Signed-off-by: saroshali-dbx <[email protected]>
Signed-off-by: saroshali-dbx <[email protected]>
@saroshali-dbx
Copy link
Contributor Author

gentle ping @roidelapluie @SuperQ :)

For context, here's the issue created for the downstream dependency - weaveworks/common#242

@SuperQ
Copy link
Member

SuperQ commented Aug 1, 2022

I thought most certificate validation has moved to verifying SANs, not CNs.

@saroshali-dbx
Copy link
Contributor Author

I thought most certificate validation has moved to verifying SANs, not CNs.

Yeah, thats the intention with SANs to be able to get around the limitation of only being able to specify a single server-name in CN. I ported this functionality from etcd -- and my organization still utilizes common-name -- but I can update this PR to check SANs first and then fallback to CNs. How does that sound?

@roidelapluie
Copy link
Member

Actually that's a good point @SuperQ , even go uses SAN's and has removed support for CN. I think we should follow go and use SANs

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Common Name is deprecated for validation. This needs to use SANs.

From RFC 2818, May 2000:

Although the use of the Common Name is existing practice, it is deprecated and Certification Authorities are encouraged to use the dNSName instead.

Chrome-based browsers dropped CN-only in 2017.

@bboreham
Copy link
Member

@saroshali-dbx are you likely to return to this, or shall we close it?

@logopk
Copy link

logopk commented Mar 17, 2023

Just a question: isn't SAN or any other DNS unusual for client certs? I usually create them with

C = DE, ST = xxx, L = xx, O = xxx, OU = xxx, [email protected]

Thus given the 3.2
SHOULD check the identity as described above

leads me to
Otherwise, the (most specific) Common Name field in the Subject field of the certificate MUST be used

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.

Further secure TLS communications
5 participants