Skip to content

Conversation

@ganskef
Copy link
Collaborator

@ganskef ganskef commented Jan 20, 2015

It's derived from the OWASP Zed Attack Proxy Project, which is licensed
under the Apache License, Version 2.0. The certificate authority is
created lazily with Bouncy Castle. The key store type is PKCS12, which
has to be implemented by every vendor. The server certificates won't be
cached, but the connection is already cached. It lacks upstream
certificate validation(!) and a proper exception handling.

It's derived from the OWASP Zed Attack Proxy Project, which is licensed
under the Apache License, Version 2.0. The certificate authority is
created lazily with Bouncy Castle. The key store type is PKCS12, which
has to be implemented by every vendor. The server certificates won't be
cached, but the connection is already cached. It lacks upstream
certificate validation(!) and a proper exception handling.
@jekh
Copy link
Collaborator

jekh commented Jan 24, 2015

I like this. It looks like it's an implementation of previous PR #171, but implemented with Bouncy Castle instead of the internal sun classes. A couple questions:

  • Generally speaking, how long does it take to generate a certificate? I'm wondering if maybe we should cache them for a short period of time.
  • Should we make the bouncycastle dependency optional in the POM, so only clients that want to use this functionality get the bouncy castle jars? (I think the idea is to eventually move this to a separate 'extras' project.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious... why mark this new class @deprecated? Is there a replacement we should use instead? The listed deprecation reason doesn't seem like a real reason to deprecate the class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious... why mark this new class @deprecated

I've modified the API and this class shows the reason for. Use
HostNameMitmManager instead of CertificateSniffingMitmManager please.

First: it's simpler to impersonate by host name. Second: long lists of
subject alternative names are truncated somewhere. A bug? But: it's
important to store the complete upstream cert to present it to the
user anyway.

Please give me both, the host name and the SSL session in the manager.

@ganskef
Copy link
Collaborator Author

ganskef commented Jan 24, 2015

  • Generally speaking, how long does it take to generate a certificate?
    I'm wondering if maybe we should cache them for a short period of time.

With my Laptop, Core 2 Duo, 2.4GHz, Linux: regulary 50 - 150ms

9339 2015-01-24 09:13:23,402 INFO [MoCuishle-ClientToProxyWorker-0]
mitm.McSslEngineSource - Impersonated github.com in 618ms
21002 2015-01-24 09:13:25,065 INFO [MoCuishle-ClientToProxyWorker-1]
mitm.McSslEngineSource - Impersonated collector.githubapp.com in 453ms
32898 2015-01-24 09:13:36,961 INFO [MoCuishle-ClientToProxyWorker-6]
mitm.McSslEngineSource - Impersonated avatars3.githubusercontent.com in
143ms
33213 2015-01-24 09:13:37,276 INFO [MoCuishle-ClientToProxyWorker-7]
mitm.McSslEngineSource - Impersonated avatars3.githubusercontent.com in
200ms
44493 2015-01-24 09:13:48,556 INFO [MoCuishle-ClientToProxyWorker-5]
mitm.McSslEngineSource - Impersonated github.com in 83ms
45493 2015-01-24 09:13:49,556 INFO [MoCuishle-ClientToProxyWorker-6]
mitm.McSslEngineSource - Impersonated avatars3.githubusercontent.com in
333ms
45638 2015-01-24 09:13:49,701 INFO [MoCuishle-ClientToProxyWorker-1]
mitm.McSslEngineSource - Impersonated avatars3.githubusercontent.com in
460ms
45766 2015-01-24 09:13:49,829 INFO [MoCuishle-ClientToProxyWorker-7]
mitm.McSslEngineSource - Impersonated avatars2.githubusercontent.com in
310ms
45784 2015-01-24 09:13:49,847 INFO [MoCuishle-ClientToProxyWorker-2]
mitm.McSslEngineSource - Impersonated avatars2.githubusercontent.com in
328ms
46124 2015-01-24 09:13:50,187 INFO [MoCuishle-ClientToProxyWorker-0]
mitm.McSslEngineSource - Impersonated cloud.githubusercontent.com in 157ms
57942 2015-01-24 09:14:02,005 INFO [MoCuishle-ClientToProxyWorker-3]
mitm.McSslEngineSource - Impersonated avatars1.githubusercontent.com in
279ms
57988 2015-01-24 09:14:02,051 INFO [MoCuishle-ClientToProxyWorker-2]
mitm.McSslEngineSource - Impersonated avatars2.githubusercontent.com in
379ms
58042 2015-01-24 09:14:02,105 INFO [MoCuishle-ClientToProxyWorker-0]
mitm.McSslEngineSource - Impersonated camo.githubusercontent.com in 349ms
58043 2015-01-24 09:14:02,106 INFO [MoCuishle-ClientToProxyWorker-6]
mitm.McSslEngineSource - Impersonated avatars1.githubusercontent.com in
403ms
58051 2015-01-24 09:14:02,114 INFO [MoCuishle-ClientToProxyWorker-1]
mitm.McSslEngineSource - Impersonated avatars0.githubusercontent.com in
361ms
58167 2015-01-24 09:14:02,230 INFO [MoCuishle-ClientToProxyWorker-4]
mitm.McSslEngineSource - Impersonated avatars3.githubusercontent.com in
525ms
58350 2015-01-24 09:14:02,413 INFO [MoCuishle-ClientToProxyWorker-7]
mitm.McSslEngineSource - Impersonated avatars3.githubusercontent.com in
682ms
65915 2015-01-24 09:14:09,978 INFO [MoCuishle-ClientToProxyWorker-4]
mitm.McSslEngineSource - Impersonated camo.githubusercontent.com in 64ms
69002 2015-01-24 09:14:13,065 INFO [MoCuishle-ClientToProxyWorker-3]
mitm.McSslEngineSource - Impersonated avatars2.githubusercontent.com in 76ms
69042 2015-01-24 09:14:13,105 INFO [MoCuishle-ClientToProxyWorker-4]
mitm.McSslEngineSource - Impersonated avatars2.githubusercontent.com in
123ms
71423 2015-01-24 09:14:15,486 INFO [MoCuishle-ClientToProxyWorker-4]
mitm.McSslEngineSource - Impersonated avatars0.githubusercontent.com in 94ms
71438 2015-01-24 09:14:15,501 INFO [MoCuishle-ClientToProxyWorker-3]
mitm.McSslEngineSource - Impersonated avatars0.githubusercontent.com in
109ms

It benefits from connection caching. High load to one site - less
impersonations. It feels good with my single user caching proxy, but I
will give it a try with a memory cache, too.

  • Should we make the bouncycastle dependency optional in the POM, ...
    Yes, but this depends the place for it. It's a draft. I've factored out
    an Authority class for modular use now, but this is obsolete if it's an
    example only.

The commons-collections was in test scope and built with Eclipse.
@ganskef
Copy link
Collaborator Author

ganskef commented Jan 24, 2015

I really think this additionally caching is obsolete. It feels not better for me. Please double check this in your environments.

A MITM manager must never be instantiated without a valid key store.
@jekh
Copy link
Collaborator

jekh commented Jan 26, 2015

I really think this additionally caching is obsolete. It feels not better for me. Please double check this in your environments.

Despite this, I see you did indeed add caching :) I like the guava cache implementation and I think it's a worthwhile addition. I did add a couple in-line comments for you to review.

I think this is a GREAT PR, thank you for putting it together! It's going to be a big help for browsermob-proxy once our littleproxy integration is complete.

Upper case static final LOG variable
Removed static final from createRootCA()
Renamed instance variables for dynamic server certificates
Cache could be given by a parameter for singleton or possibly null
@ganskef
Copy link
Collaborator Author

ganskef commented Jan 26, 2015

... I did add a
couple in-line comments for you to review.
Sorry, what is that - I mean where is that?

A proxy must never install a manager without a valid certificate
authority, but exceptions with dynamic certificates could be handled
differentially.

Enhanced documentation
@jekh
Copy link
Collaborator

jekh commented Feb 8, 2015

@ganskef -- it looks like the bcprov-jdk16 artifact is pretty out-of-date (Feb 2011) and not actively maintained. bcprov-jdk15on works with Java 1.5+ and is actively maintained (latest release Jul 2014). Would it be possible to update this PR to work with the jdk15on versions of bouncy castle?

@jekh
Copy link
Collaborator

jekh commented Feb 9, 2015

Also, could you add some unit tests? That would have the secondary benefit of showing people how to use the new functionality.

@ganskef ganskef changed the title HTTPS impersonation working with LittleProxy HTTPS and offline cache working with LittleProxy Feb 28, 2015
@myleshorton
Copy link
Contributor

Thanks @ganskef! I agree with @jekh that a quick unit test would be super helpful. You mind adding that to the PR?

@ganskef
Copy link
Collaborator Author

ganskef commented Mar 1, 2015

Hello Myle,

yes, I'm working on it. Its not so easy. I need a SSL enabled server and
client with proper certificate handling. I'm confused about the tests in
LittleProxy and its dependencies.

I'm thinking about more composition than inheritance. I plan tests for
this directly based on Netty instead of other frameworks.

Regards Frank

Am 01.03.2015 um 07:47 schrieb myleshorton:

Thanks @ganskef https://github.com/ganskef! I agree with @jekh
https://github.com/jekh that a quick unit test would be super helpful.
You mind adding that to the PR?


Reply to this email directly or view it on GitHub
#174 (comment).

@jekh
Copy link
Collaborator

jekh commented Mar 1, 2015

Hi @ganskef -- I see the MITM functionality has been removed from this PR, but where did it go? I'm very excited about that and I would love to help you test it :)

Regarding the rest of the PR, I wanted to ask: what functionality is it implementing? I see it is now titled "HTTPS and offline cache" (instead of the previous title "HTTPS impersonation"), but what exactly is the offline cache doing? What is the use case you have in mind?

Since the purpose of this PR has completely changed, could you at least squash the commits related to the MITM impersonation, since they are not part of this change anymore? The extra noise makes understanding the offline caching change a bit confusing.

For future reference, if you want to implement a different feature, it's usually easiest and cleanest to just create a new PR and close the old one (if you no longer want to merge it).

The purpose is to store traffic while Online and spool it in an Offline
mode.
@ganskef
Copy link
Collaborator Author

ganskef commented Mar 13, 2015

Hello Jason,

this PR targets the following issues:

  1. Chunk aggregation and and content expansion is disabled when
    detecting a CONNECT. This disables it for MITM use. (see PR Enable aggregation and inflation for filtering with mitm. #175)
  2. A handshake with the proxy initiates always a connection to the
    server. Therefore MITM while offline is impossible. I've written a test
    now. What do you think?
  3. A MitmManager and EngineSource implements a full Certificate
    Authority to sign server certificates on the fly. It can be imported in
    the browser as a trusted CA. It is moved caused by Move extras package into its own project #173, but it could
    moved back again. I have it in my daily use and it's working fine, but
    this proxy depends on 1 and 2, too.

What should I do? I'm helpless. Please, give me some suggestions.

Should I close this request and create new PRs more dedicated?

Should I delete the forked LittleProxy to get a clean master?

Should I create a branch for each feature like I've done in PR #175?

Regards Frank

The hosts must be unreachable to show, what happens. Now it's necessary
to bootstrap an AdressResolver to suppress the server handshake.
@ganskef
Copy link
Collaborator Author

ganskef commented Mar 13, 2015

@jekh sorry, I've forgotten an issue:

  1. An offline proxy needs an additional parameter host name in MitmManager#clientSslEngineFor. The host name is available in the proxy to client connection, without a server connection. The MITM server certificate will be allways created with the host name instead of the original given common name or the subject alternative names. The SSLSession should be available even though, to retrieve the upstream certificate.

@ganskef ganskef mentioned this pull request May 9, 2015
ganskef added 6 commits May 12, 2015 19:08
Proxy to server connections needs peer informations to connect Server
Name Indication (SNI) enabled sites. The client has to send an extension
server_name: [host_name: developer.chrome.com] for example within the
handshake.
ganskef added 2 commits May 31, 2015 22:43
Conflicts:
	src/main/java/org/littleshoot/proxy/impl/ProxyToServerConnection.java
…isk#210

Conflicts:
	src/main/java/org/littleshoot/proxy/MitmManager.java
	src/main/java/org/littleshoot/proxy/SslEngineSource.java
	src/main/java/org/littleshoot/proxy/extras/SelfSignedMitmManager.java
	src/main/java/org/littleshoot/proxy/impl/ProxyToServerConnection.java
@zerkz
Copy link

zerkz commented Jun 30, 2015

nice work @ganskef . I think your https://github.com/ganskef/LittleProxy-mitm project should be atleast mentioned in the MITM part of the readme of this project. What do you think @jekh?

ganskef added 7 commits July 17, 2015 13:26
Not closing client connections even when all servers have disconnected
except for sslEngine is not null, elsewhere it breaks Man-In-The-Middle.
 - Current version (SNAPSHOT)
 - some common use cases
 - subscribe forum hint
 - removed incomplete revision history
With adamfisk#122 context of the client connection only was made available to
filters. It makes it possible to access the pipeline change codecs and
decoders, if needed. This commit provides the server context on
connection succeded too.
@ganskef
Copy link
Collaborator Author

ganskef commented Jul 18, 2015

I will create a separate branch for this, since I've started to mix incidents in the master. So I'm going to close this pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants