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

Tls Handshake Layout Render #802

Merged
merged 6 commits into from
Jul 3, 2022
Merged

Conversation

bakgerman
Copy link
Contributor

Add a single layout render, with an enum to specify which property to render, with unit tests.
Modelled upon ProcessInfoLayoutRenderer.
Hopefully better than 7 separate layout renderers.

@snakefoot
Copy link
Contributor

snakefoot commented Jun 23, 2022

Thank you for the pull-request and the unit-tests.

And you have been missing this layout-renderer while developing ASP.NET Core applications?

@bakgerman
Copy link
Contributor Author

Thank you for the pull-request and the unit-tests.

And you have been missing this layout-renderer while developing ASP.NET Core applications?

Basically I had to prove the details of the TLS being used to the security team, I don't know why they think the application can affect the chosen handshake. But I have to prove the application innocent.

@bakgerman
Copy link
Contributor Author

I examined other interfaces available in the HttpContext FeatureCollection and did some more LayoutRenders, but I am not sure if they are good for NLog or not. I found the following, but these only work for ASP_NET_CORE3 case in NLog.Web:

Http/2 request trailer
Http/2 response trailer
DoNotTrack
OAuth bearer token
http compression mode
Server Variables - server variables are available in ASP.NET Core, I had to use this to log AppPool of my application to prove they assign my application to wrong app pool. That one might be good.

@snakefoot
Copy link
Contributor

Again. I'm an ignorant to ASP.NET Core applications, so my observations might not be true:

  • Http/2 request trailer - These are probably part of the request-body, so requires middleware.
  • Http/2 response trailer - These are probably part of the response-body, so requires middleware.
  • DoNotTrack - Sounds good.
  • http compression mode - Sounds good.
  • Server Variables - server variables are available in ASP.NET Core - Sounds good.
  • OAuth bearer token - Might be useful to diagnose authentication-issue.

@bakgerman
Copy link
Contributor Author

I see, we agree in our opinion. :)
I thought a lot of it was useful only for internal Microsoft developer or for middle ware developer.

I am go on holiday today for a week but I will finish the unit tests and submit a pull request for the ones we think are useful for NLog.

Regards,
Burak

@snakefoot
Copy link
Contributor

snakefoot commented Jun 25, 2022

Implment review correections
@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2022

Codecov Report

Merging #802 (840483f) into master (61f8567) will increase coverage by 1%.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #802   +/-   ##
=====================================
+ Coverage      68%    70%   +1%     
=====================================
  Files          61     62    +1     
  Lines        1142   1146    +4     
  Branches      287    289    +2     
=====================================
+ Hits          779    797   +18     
+ Misses        239    225   -14     
  Partials      124    124           
Impacted Files Coverage Δ
src/Shared/Internal/HttpContextExtensions.cs 68% <ø> (+14%) ⬆️
...enderers/AspNetRequestIsWebSocketLayoutRenderer.cs 100% <0%> (ø)
...equestWebSocketRequestedProtocolsLayoutRenderer.cs 78% <0%> (ø)
...enderers/AspNetResponseHasStartedLayoutRenderer.cs 100% <0%> (ø)
...utRenderers/AspNetRequestDurationLayoutRenderer.cs 64% <0%> (+7%) ⬆️
...rs/AspNetRequestClientCertificateLayoutRenderer.cs 57% <0%> (+7%) ⬆️
...ared/LayoutRenderers/AspNetRequestContentLength.cs 100% <0%> (+29%) ⬆️
...enderers/AspNetRequestContentTypeLayoutRenderer.cs 71% <0%> (+71%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61f8567...840483f. Read the comment docs.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 2, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@snakefoot snakefoot merged commit a35f036 into NLog:master Jul 3, 2022
@snakefoot
Copy link
Contributor

Thank you again for another nice contribution.

@snakefoot snakefoot added this to the 5.1.0 milestone Jul 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants