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

DefaultCertificateSelector should log a debug message if no matching certificates are found. #222

Open
ryancdotorg opened this issue Mar 21, 2023 · 3 comments
Labels
feature request Request for new feature or functionality

Comments

@ryancdotorg
Copy link

ryancdotorg commented Mar 21, 2023

What would you like to have changed?

DefaultCertificateSelector in handshake.go should log a debug message if no matching certificates are found.

There is currently just a comment for that case, reading:

// all matching certs are expired or incompatible, oh well

Why is this feature a useful, necessary, and/or important addition to this project?

It's

  1. Trivial to add.
  2. Having it would ease debugging - I spent two hours wondering why things weren't working because I didn't bother adding SANs to my test certificates, and a debug message would have helped me find the problem sooner.

What alternatives are there, or what are you doing in the meantime to work around the lack of this feature?

I don't think there's any meaningful alternatives or workarounds.

Please link to any relevant issues, pull requests, or other discussions.

I have documented my mistakes so that others needn't repeat them. caddyserver/caddy#5450

@ryancdotorg ryancdotorg added the feature request Request for new feature or functionality label Mar 21, 2023
@francislavoie
Copy link
Member

francislavoie commented Mar 21, 2023

That would get logged on every TLS request to an unknown domain, so it might be pretty noisy. There's already a TLS handshake error at debug level, adding another there would mean two logs for every bad request (including from bots).

I think there's probably a better place to put it. We should probably more loudly write an error log on startup when a cert with no SAN is provided.

@ryancdotorg
Copy link
Author

logged on every TLS request to an unknown domain

I was suggesting a message be logged at debug level only, which already has multiple messages for every TLS connection, so I don't think this would be an issue.

We should probably more loudly write an error log on startup when a cert with no SAN is provided.

That would be helpful - if someone is trying to generate a self-signed certificate for testing, they'll likely find instructions for doing so with OpenSSL - without SANs.

@mholt
Copy link
Member

mholt commented May 6, 2023

@ryancdotorg Sorry for the late reply, but am looking into this. I think we're already logging when the cert has no subjects. When you load your SAN-less certificate, there should be a debug log emitted with message "added certificate to cache" -- and it should list the subjects next to it on the same line. On a relevant certificate, what does that log line say?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for new feature or functionality
Projects
None yet
Development

No branches or pull requests

3 participants