Log Client Certificate Identity Information in Access Logs#145
Conversation
geigerj0
changed the title
| if r.TLS != nil && len(r.TLS.PeerCertificates) > 0 { | ||
| indexLeafCertConnectionIsVerifiedAgainst := 0 // see also https://github.com/golang/go/blob/e929fb78e47dc191a402d34ca949d2e0c67e31b8/src/crypto/tls/common.go#L281-L282 | ||
| cert := r.TLS.PeerCertificates[indexLeafCertConnectionIsVerifiedAgainst] | ||
|
|
||
| lagerData["peer_cert_subject_common_name"] = cert.Subject.CommonName | ||
| lagerData["peer_cert_subject_organizational_unit"] = cert.Subject.OrganizationalUnit | ||
| lagerData["peer_cert_subject_organization"] = cert.Subject.Organization | ||
|
|
||
| lagerData["peer_cert_issuer_common_name"] = cert.Issuer.CommonName | ||
| lagerData["peer_cert_issuer_organizational_unit"] = cert.Issuer.OrganizationalUnit | ||
| lagerData["peer_cert_issuer_organization"] = cert.Issuer.Organization | ||
| } | ||
|
|
||
| return lagerData |
There was a problem hiding this comment.
FYI: this causes the LogWrap-middleware to always log peer cert information. however, the related log messages are being emitted with log level DEBUG and thus will more or less never be visible because no one runs this with DEBUG productively
bbs/handlers/middleware/middleware.go
Lines 49 to 54 in e5644ae
| lagerData["peer_cert_subject_common_name"] = cert.Subject.CommonName | ||
| lagerData["peer_cert_subject_organizational_unit"] = cert.Subject.OrganizationalUnit | ||
| lagerData["peer_cert_subject_organization"] = cert.Subject.Organization | ||
|
|
||
| lagerData["peer_cert_issuer_common_name"] = cert.Issuer.CommonName | ||
| lagerData["peer_cert_issuer_organizational_unit"] = cert.Issuer.OrganizationalUnit | ||
| lagerData["peer_cert_issuer_organization"] = cert.Issuer.Organization |
There was a problem hiding this comment.
we can definitely discuss these key names (peer_cert_*), maybe you have a better idea
cf-foundation-community-automation
Bot
moved this to Inbox
in Application Runtime Platform Working Group
| When("request has TLS peer certificates", func() { | ||
| BeforeEach(func() { | ||
| accessLogger = lagertest.NewTestLogger("") | ||
| accessLogger.RegisterSink(lager.NewWriterSink(GinkgoWriter, lager.INFO)) // peer cert information should be logged at INFO level |
There was a problem hiding this comment.
it is very important to test that the log messages appear also for INFO-logging because this is the level the access logger is being set up with:
Line 199 in e5644ae
geigerj0
changed the title
ameowlia
left a comment
ameowlia
left a comment
There was a problem hiding this comment.
Hi @geigerj0 , Thank you for submitting this PR and for the helpful comments to contextualize it.
I am slightly worried about the additional logging this will cause for every access log, but given that turning on access logs is already behind a config flag, I think it is okay.
If there are complains in the future about the size of this access log then we might need to put this behind its own config flag.
github-project-automation
Bot
moved this from Inbox
to Pending Merge | Prioritized
in Application Runtime Platform Working Group
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Done
in Application Runtime Platform Working Group
2 participants
Problem
BBS currently does not include TLS peer certificate information in its access logs, making it difficult to reliably identify authenticated clients. At the moment, only the client IP address is logged, while no certificate-based identity information about the caller is recorded.
bbs/handlers/middleware/middleware.go
Line 26 in e5644ae
Solution
This PR enhances the
LogWrapmiddleware to include TLS peer certificate information in request logs whenever such information is available.If an access logger is configured via
access_log_path, the TLS peer certificate details are logged at theINFOlevel and therefore written to the access log.Backward Compatibility
Breaking Change? No
Contact Me
Feel free to reach out directly via Slack: https://cloudfoundry.slack.com/team/U0578H2V37D