Add tracing to the Linux X509Chain implementation #65860
Conversation
bartonjs
requested review from
davidfowl,
vcsjones,
GrabYourPitchforks and
stephentoub
|
Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones Issue DetailsThe tracing is heavily focused on transient behaviors, like AIA/CRL/OCSP downloads and cache interactions. Things like applying ApplicationPolicy aren't really traced because that can be "easily" evaluated by looking at the certificates in isolation. Example tracing, from the microsoft.com certificate: Fixes #40445.
|
...raphy/src/System/Security/Cryptography/X509Certificates/OpenSslCertificateAssetDownloader.cs
Outdated
Show resolved
Hide resolved
...raphy/src/System/Security/Cryptography/X509Certificates/OpenSslCertificateAssetDownloader.cs
Show resolved
Hide resolved
...ryptography/src/System/Security/Cryptography/X509Certificates/OpenSslX509ChainEventSource.cs
Outdated
Show resolved
Hide resolved
| } | ||
| else | ||
| { | ||
| OpenSslX509ChainEventSource.Log.NonHttpCdpEntry(name.Uri); |
There was a problem hiding this comment.
It might be "http" but failed to parse because of some issue later in the uri. Is that ok?
There was a problem hiding this comment.
PerfView would just see it as NonHttpCdpEntry / http://test..?q=w.
Something that takes the message into account would see Skipping CDP entry 'http://test..?q=w' because the protocol is not HTTP., which seems less than ideal, but not too far off the mark.
| internal bool ShouldLogElementStatuses() | ||
| { | ||
| return IsEnabled(EventLevel.Verbose, EventKeywords.None); | ||
| } |
There was a problem hiding this comment.
@marek-safar, will this helper confuse the linker and its ability to remove EventSource-related code based on guards, or is this ok?
| } | ||
| } | ||
|
|
||
| OpenSslX509ChainEventSource.Log.NoMatchingCdpEntry(); |
There was a problem hiding this comment.
FWIW, we've typically guarded all EventSource calls, even parameterless onces, with IsEnabled() checks. Part of that has been historical, and part of that has been in support of the linker being able to remove everything that's only referenced inside such a guard. @marek-safar should comment on whether this is ok.
There was a problem hiding this comment.
And here I thought that was an artifact of older patterns.
So the correct style, for dotnet/runtime, at least, is an IsEnabled check on both the outside and the inside of the method? (I feel like EventSource's documentation said to have an IsEnabled check for each method, presumably to avoid things like creating the params array when it doesn't matter) Or do we do them outside-only?
There was a problem hiding this comment.
Yep, linker will have right now difficulties propagating this information. The best is to call directly methods that are substituted when event source is disabled
ghost
locked as resolved and limited conversation to collaborators
The tracing is heavily focused on transient behaviors, like AIA/CRL/OCSP downloads and cache interactions.
Things like applying ApplicationPolicy aren't really traced because that can be "easily" evaluated by looking at the certificates in isolation.
Example tracing, from the microsoft.com certificate:
Fixes #40445.