[WIP] Allow trusted proxies to forward client certificate fingerprints (#1430) #1478
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1478 +/- ##
=========================================
- Coverage 37.63% 36.54% -1.1%
=========================================
Files 120 126 +6
Lines 23862 30838 +6976
Branches 93 93
=========================================
+ Hits 8981 11270 +2289
- Misses 14840 19527 +4687
Partials 41 41
Continue to review full report at Codecov.
|
include/znc/HTTPSock.h
Outdated
| @@ -86,6 +86,7 @@ class CHTTPSock : public CSocket { | |||
| const CString& GetURI() const; | |||
| const CString& GetURIPrefix() const; | |||
| bool IsPost() const; | |||
| long GetPeerFingerprint(CS_STRING& sResult) const override; | |||
src/HTTPSock.cpp
Outdated
| @@ -181,6 +188,12 @@ void CHTTPSock::ReadLine(const CString& sData) { | |||
| // X-Forwarded-For list in both cases use it as the endpoind | |||
| m_sForwardedIP = sIP; | |||
| } | |||
| } else if (sName.Equals("X-Forwarded-CertFP:")) { | |||
| bool bTrusted = IsTrustedProxy(GetRemoteIP()); | |||
| if(bTrusted) { | |||
There was a problem hiding this comment.
space missing
you can use clang-format to fix style with a single keypress, and not care about indentation and other things manually
src/HTTPSock.cpp
Outdated
| @@ -181,6 +188,12 @@ void CHTTPSock::ReadLine(const CString& sData) { | |||
| // X-Forwarded-For list in both cases use it as the endpoind | |||
| m_sForwardedIP = sIP; | |||
| } | |||
| } else if (sName.Equals("X-Forwarded-CertFP:")) { | |||
| bool bTrusted = IsTrustedProxy(GetRemoteIP()); | |||
There was a problem hiding this comment.
if (IsTrusted...) is fine. The new variable doesn't add anything here.
src/HTTPSock.cpp
Outdated
| bool bTrusted = IsTrustedProxy(GetRemoteIP()); | ||
| if(bTrusted) { | ||
| m_sForwardedCertFP = sLine.Token(1, true); | ||
| DEBUG("Got a forwarded CertFP '" << m_sForwardedCertFP << "'"); |
There was a problem hiding this comment.
You might want to output a socket name here.
| @@ -181,6 +188,12 @@ void CHTTPSock::ReadLine(const CString& sData) { | |||
| // X-Forwarded-For list in both cases use it as the endpoind | |||
| m_sForwardedIP = sIP; | |||
| } | |||
| } else if (sName.Equals("X-Forwarded-CertFP:")) { | |||
There was a problem hiding this comment.
I can't find anything about this name.
There was a problem hiding this comment.
Yeah, it's not standardized anywhere. X-Forwarded-For is standardized by convention but as far as I know there isn't anything similar to TLS fingerprints, as this sort of thing isn't as widely done. This would have to go in documentation somewhere.
|
I'll work on a test later; I'm having some trouble figuring out a good strategy for testing this since things like |
|
Check out integration test, it uses network.
With virtual function it'd be cleaner, yes. But what hash algorithm is used
by this new http header? I wonder whether it's possible to change the
algorithn somehow. I am a bit wary that result of this function changes
over time but probably that should be fine.
Also it somewhat turns hash into password. Previously, if trusted proxy is
hacked, attacker could only circumvent whitelist of ips, and fail2ban. But
now would gain complete access to a user account if they learnt
fingerprint, which is not supposed to be secret.
23 дек. 2017 г. 8:57 PM пользователь "Empty String" <
notifications@github.com> написал:
I'll work on a test later; I'm having some trouble figuring out a good
strategy for testing this since things like IsTrustedProxy are
network-dependent.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1478 (comment)>, or mute the
thread
<https://github.com/notifications/unsubscribe-auth/AAT15LwvxPXnrh4RuExU-IM8IH9LgoNFks5tDWlJgaJpZM4RLjT2>
.
|
|
The hash algorithm is sha1 and it can't be changed, but this is the case with normal certauth currently also. That's a good point about a compromised TrustedProxy. How about a global config flag that enables/disables this behavior? I'll take a look at the integration test stuff, that sounds like what I want. |
a8f9c7b to
5f8fb2c
Compare
…#1430) These changes allow trusted proxies to set a header overriding the detected client certificate fingerprint. This allows webadmin behind a HTTPS-supporting proxy to correctly work with the certauth module.
5f8fb2c to
ec36715
Compare
These changes allow trusted proxies to set a header overriding the detected client certificate fingerprint. This allows webadmin behind a HTTPS-supporting proxy to correctly work with the certauth module. This would fix #1430.
WIP because I'm really uncomfortable with the
dynamic_castthat's used here. The alternatives are to change types in a bunch of other places, or just makeCsock::GetPeerFingerprintvirtual. I think the last one is the best solution, and if you agree I can get a PR off to CSocket and fix this one up.Aside from the nasty
dynamic_castI'd be interested in any feedback.