[WIP] Allow trusted proxies to forward client certificate fingerprints (#1430) #1478

an-empty-string · 2017-12-23T02:46:03Z

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_cast that's used here. The alternatives are to change types in a bunch of other places, or just make Csock::GetPeerFingerprint virtual. 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_cast I'd be interested in any feedback.

codecov · 2017-12-23T03:40:29Z

Codecov Report

Merging #1478 into master will decrease coverage by 1.09%.
The diff coverage is 3.22%.

@@            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

Impacted Files	Coverage Δ
include/znc/HTTPSock.h	`100% <ø> (ø)`	⬆️
modules/certauth.cpp	`1.75% <0%> (-0.41%)`	⬇️
include/znc/Socket.h	`42.02% <0%> (-7%)`	⬇️
src/HTTPSock.cpp	`40.86% <5.26%> (-12.02%)`	⬇️
include/znc/MD5.h	`75% <0%> (-25%)`	⬇️
modules/keepnick.cpp	`40.16% <0%> (-16.16%)`	⬇️
modules/samplewebapi.cpp	`73.91% <0%> (-15.57%)`	⬇️
src/Client.cpp	`64.2% <0%> (-13.06%)`	⬇️
src/WebModules.cpp	`47.29% <0%> (-12.36%)`	⬇️
... and 98 more

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 15ccaca...ec36715. Read the comment docs.

DarthGandalf

Tests? :)

DarthGandalf · 2017-12-23T09:25:08Z

include/znc/HTTPSock.h

@@ -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;


It's CString here.

DarthGandalf · 2017-12-23T09:28:14Z

src/HTTPSock.cpp

@@ -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) {


space missing
you can use clang-format to fix style with a single keypress, and not care about indentation and other things manually

DarthGandalf · 2017-12-23T09:28:27Z

src/HTTPSock.cpp

@@ -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 (IsTrusted...) is fine. The new variable doesn't add anything here.

DarthGandalf · 2017-12-23T09:29:39Z

src/HTTPSock.cpp

+        bool bTrusted = IsTrustedProxy(GetRemoteIP());
+        if(bTrusted) {
+            m_sForwardedCertFP = sLine.Token(1, true);
+            DEBUG("Got a forwarded CertFP '" << m_sForwardedCertFP << "'");


You might want to output a socket name here.

DarthGandalf · 2017-12-23T09:32:35Z

src/HTTPSock.cpp

@@ -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:")) {


I can't find anything about this name.

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.

an-empty-string · 2017-12-23T20:57:45Z

I'll work on a test later; I'm having some trouble figuring out a good strategy for testing this since things like GetRemoteIP are network-dependent.

DarthGandalf · 2017-12-24T09:53:37Z

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> .

an-empty-string · 2017-12-25T03:27:44Z

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.

…#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.

DarthGandalf reviewed Dec 23, 2017

View reviewed changes

an-empty-string force-pushed the master branch 2 times, most recently from a8f9c7b to 5f8fb2c Compare February 18, 2018 23:21

an-empty-string added 2 commits February 18, 2018 17:23

fix review issues

ec36715

an-empty-string force-pushed the master branch from 5f8fb2c to ec36715 Compare February 18, 2018 23:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Allow trusted proxies to forward client certificate fingerprints (#1430) #1478

[WIP] Allow trusted proxies to forward client certificate fingerprints (#1430) #1478

an-empty-string commented Dec 23, 2017 •

edited

codecov bot commented Dec 23, 2017 •

edited

DarthGandalf left a comment

DarthGandalf Dec 23, 2017

DarthGandalf Dec 23, 2017

DarthGandalf Dec 23, 2017

DarthGandalf Dec 23, 2017

DarthGandalf Dec 23, 2017

an-empty-string Dec 23, 2017

an-empty-string commented Dec 23, 2017 •

edited

DarthGandalf commented Dec 24, 2017 via email

an-empty-string commented Dec 25, 2017

[WIP] Allow trusted proxies to forward client certificate fingerprints (#1430) #1478

Are you sure you want to change the base?

[WIP] Allow trusted proxies to forward client certificate fingerprints (#1430) #1478

Conversation

an-empty-string commented Dec 23, 2017 • edited

codecov bot commented Dec 23, 2017 • edited

Codecov Report

DarthGandalf left a comment

Choose a reason for hiding this comment

DarthGandalf Dec 23, 2017

Choose a reason for hiding this comment

DarthGandalf Dec 23, 2017

Choose a reason for hiding this comment

DarthGandalf Dec 23, 2017

Choose a reason for hiding this comment

DarthGandalf Dec 23, 2017

Choose a reason for hiding this comment

DarthGandalf Dec 23, 2017

Choose a reason for hiding this comment

an-empty-string Dec 23, 2017

Choose a reason for hiding this comment

an-empty-string commented Dec 23, 2017 • edited

DarthGandalf commented Dec 24, 2017 via email

an-empty-string commented Dec 25, 2017

an-empty-string commented Dec 23, 2017 •

edited

codecov bot commented Dec 23, 2017 •

edited

an-empty-string commented Dec 23, 2017 •

edited