This repository has been archived by the owner on Feb 27, 2023. It is now read-only.
feature: Hijack HTTPS by generating leaf TLS certs on the fly issued by user provided CA. #1181
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
We found this is your first time to contribute to Dragonfly, @YanzheL |
YanzheL
force-pushed
the
feature-https-mitm
branch
2 times, most recently
from
b9475ff
to
e40b16a
Compare
Codecov Report
@@ Coverage Diff @@
## master #1181 +/- ##
==========================================
- Coverage 48.27% 47.89% -0.38%
==========================================
Files 116 117 +1
Lines 7263 7320 +57
==========================================
Hits 3506 3506
- Misses 3477 3534 +57
Partials 280 280
Continue to review full report at Codecov.
|
YanzheL
changed the title
|
@YanzheL Thanks a lot for your PR ! This has been a known issue we haven't yet fix. The current implementation indeed requires clients to explicitly ignore certificate errors. Adding a CA to the system cert store is the right way to do it. I'll review the code later today, meanwhile would you please:
|
YanzheL
force-pushed
the
feature-https-mitm
branch
from
e40b16a
to
d3e6b2a
Compare
|
Thanks for your reply. I wrote a simple guide to test this feature. How to use dfclient with custom CAGenerate Self-Signed CAI recommend using CFSSL to do this step. For user's convenience, we can implement CA auto-generation in future. If you do this with OpenSSL, please make sure the x509 Key Usage Extension contains
Now we have a self-signed CA key-pair Configure dfclient to use this CAproxies:
- regx: blobs/sha256.*
hijack_https:
cert: /path/to/your/ca.crt
key: /path/to/your/ca.key
hosts: # We test some non-dockerhub HTTPS registries here.
- regx: 'quay.io'.
- regx: 'registry.gitlab.com'
- regx: 'gcr.io'Configure
|
inoc603
reviewed
Jan 20, 2020
In fact, this error is not related to this PR. Please ignore it. |
YanzheL
force-pushed
the
feature-https-mitm
branch
from
fdfd03a
to
f5ee1fb
Compare
inoc603
reviewed
Feb 1, 2020
YanzheL
force-pushed
the
feature-https-mitm
branch
2 times, most recently
from
5d34003
to
840e446
Compare
|
LGTM Since this change is backward compatible, I think we can merge this PR now. @YanzheL would you please rebase the 3 commits into one and add a proper commit message. Documentation and automatic CA generation are still needed though, I'll open an issue to track them. Thanks for the great work! |
YanzheL
force-pushed
the
feature-https-mitm
branch
from
840e446
to
7d351b4
Compare
…rovided CA Signed-off-by: YanzheL <lee.yanzhe@yanzhe.org>
YanzheL
force-pushed
the
feature-https-mitm
branch
from
7d351b4
to
96c3c82
Compare
|
Thank you for reviewing. These commits are squashed now. |
sungjunyoung
pushed a commit
to sungjunyoung/Dragonfly
that referenced
this pull request
May 8, 2022
Signed-off-by: Gaius <gaius.qi@gmail.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: YanzheL lee.yanzhe@yanzhe.org
Ⅰ. Why do you propose this PR
The current implementation of HTTPS hijacking is simple but not correct.
It uses user provided TLS server cert
df.crtanddf.keyto decrypt HTTPS connection. However, this cert cannot be a CA, which means every TLS connection is encrypted by the same cert from user point of view.This is not a standard behavior of a HTTPS Man-In-The-Middle proxy, and will cause various issues.
The cert used by HTTPS hijacking cannot be automatically verified by applications because the Common Name (or Server-Alternative-Names) is always same and it doesn't match the host of every connection. So user have to configure their applications manually to force-ignore the TLS verification.
This prevents
dfclientbe used as a general system-level HTTPS proxy without affecting user applications.Some applications cannot be configured to trust a specific TLS cert or ignore TLS verification error (e.g. Google Chrome). Instead, the only way to achieve it is to configure them to trust the CA, or add the CA to system trust store.
II. Describe what this PR did
If
df.crtanddf.keyis a CA key-pair, thendfclientuse it to issue leaf TLS certs for every connection whose host matches pre-configured hijacking rules.User can either add the CA to system trust store, or configure individual application to trust it.
Since the common name of leaf cert is set as the target host, the connection will be verified by user application automatically as normal.
If
df.crtanddf.keyis not a CA key-pair, the behavior ofdfclientis same as the old way: this cert is used in hijacking instead of generating new certs per connection.So this PR is fully backward-compatible and will NOT break user applications.
ⅡI. Does this pull request fix one issue?
Potential issues are stated above.
IV. Potential use cases
Maybe now we can cache HTTPS docker registries (Probably fixes Why does Dragonfly can not support HTTPS mirror repositories well？ #525).
The dfdaemon acts as a decrypting MITM HTTPS proxy, so it can 'see' the request body of docker image pull requests to remote private HTTPS registries. If we can see the body, then we can cache it as well.
Generic HTTPS caching proxy, just like squid. Cache anything in a distributed way, and not just for HTTP contents. We can also use dfdaemon to speed up normal webpage loading in browser.
V. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
I'm working on unit tests, but currently I don't have much time....
For now, I just tested this feature in container, and everything seems good.
VI. Describe how to verify it
Prepare a self-signed CA with private key.
Configure dfdaemon to use this key pair, and also configure the hijack rules, proxy rules.
Setup dfdaemon as your system's http(s) proxy.
Check the TLS cert return by target site.
This command will report that the TLS cert of
alibaba.comis signed by your CA.The dfdaemon log will indicate that it is downloading png files of
alibaba.comⅤII. Special notes for reviews
This PR reuses CA's private key (and signature algorithm) to generate per-connection certs. I think it can save key-generation overhead and there is no need to use new TLS private key for every connection since the generated cert is temporal (Default valid time is 24 hours).
As for security, if the private key of CA is leaked someway, generating new private key for every connection will not improve security (maybe???). So reusing CA private key does not bring much security issues.
If this is not appropriate, I can change it.