SAN extension required for TLS interception certificate generation

[Benouare]

Hi,

When using locally proxy.py, I saw that, with FF, everything looks good, but with Brave/Chrome that was not the case.
Thx of the console of chrome, it says that there is currently two issues on our certificates :

• that was in sha1, not enought for chrome -> it needs sha256,
• the certificate must have SAN information (subectAltName)

This PR, should solved this issue.
Regarding the code, the main problem was to generate on the fly certs with the SAN information.
I tried billions of time & differents solutions, but I was not able to make it on the fly.
Currently, I am generating and consuming a file, before to remove it. I think, it will make a lot of I/O access, and should be powerless...
So, I am waiting your comment to find the best solution.

Ressources :
https://www.fomfus.com/articles/how-to-generate-self-signed-certificates-with-san-subject-alternative-name

Edit :
The 'solution' was to put something like that in the signing process to add the SAN on the fly, but that's not working with python (but working well in direct shell)

'-extfile','<(printf "[SAN]\nsubjectAltName=DNS:CurrentTld")'

But that doesnt work at all

Edit 2 : the base branch is the one for the #258, hope that's not a problem for you.

[abhinavsingh]

@Benouare I think we must start to use pki.py utilities within the interception code for generating and signing CSR requests. See test_pki.py for some usage information.

While pki.py was updated to comply with latest SSL cert requirements, I think hardcoded openssl usages within interception code might still be complying with cert requirements (specially the extensions part of cert).

[Benouare]

@Benouare I think we must start to use pki.py utilities within the interception code for generating and signing CSR requests. See test_pki.py for some usage information.

While pki.py was updated to comply with latest SSL cert requirements, I think hardcoded openssl usages within interception code might still be complying with cert requirements (specially the extensions part of cert).

I agree, I will make the corrections.

[Benouare]

So, I have made some changes.
I push the branch but it's not the final 'release' version of it : we need to talk about it, and i would like your feedback.
I have to delete some part of the code to generate the certificates 'in the good way'.
I tried to use the existants pki methods to generate certs, but that doesnt work, so I do that.
About the test, I add some, but there is one that is not working anymore. Dont get why...
Let me know what you think about it.

[abhinavsingh]

Sure thing, I'll take a look at it tonight and get back to you. Thank you!!!

…

On Sat, Jan 11, 2020, 3:58 PM Benouare ***@***.***> wrote: So, I have made some changes. I push the branch but it's not the final 'release' version of it : we need to talk about it, and i would like your feedback. I have to delete some part of the code to generate the certificates 'in the good way'. I tried to use the existants pki methods to generate certs, but that doesnt work, so I do that. About the test, I add some, but there is one that is not working anymore. Dont get why... Let me know what you think about it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#261?email_source=notifications&email_token=AAA6Y4IH5WODP3UTZGMIQRTQ5JMJTA5CNFSM4KFELVV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIWN2CY#issuecomment-573365515>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA6Y4KB4RK3PHQJX6SUWXLQ5JMJTANCNFSM4KFELVVQ> .

[abhinavsingh]

Hi @Benouare ,

I am wondering why do we need new gen_web_csr and gen_web_cert methods, doesn't existing methods in pki.py already suffice? We already used them previously to update our Makefile.

I am thinking of putting those 4 loc that you added under gen_web to replace current openssl commands run directly during interception. Wdyt?

Also, let's separate out your PR's. Send out a separate one for UUID updates, we should be able to check that in sooner. Then let's continue on this PR for cert generation fixes.

[Benouare]

I am wondering why do we need new gen_web_csr and gen_web_cert methods, doesn't existing methods in pki.py already suffice? We already used them previously to update our Makefile.

I did'nt succeed to use the methods in the pki, to generate certs that can be used by ff/chrome & others. It's for that reason that I have put some new methods.

I am thinking of putting those 4 loc that you added under gen_web to replace current openssl commands run directly during interception. Wdyt?

I don't get what you mean. The gen_web is currently used to replace the 'old' way to generate the certificate during the interception.

Also, let's separate out your PR's. Send out a separate one for UUID updates, we should be able to check that in sooner. Then let's continue on this PR for cert generation fixes.

Get it.

[abhinavsingh]

I am wondering why do we need new gen_web_csr and gen_web_cert methods, doesn't existing methods in pki.py already suffice? We already used them previously to update our Makefile.

I did'nt succeed to use the methods in the pki, to generate certs that can be used by ff/chrome & others. It's for that reason that I have put some new methods.

I am trying to understand why can't we modify existing pki.py methods to make them compatible with FF/Chrome. As of today, pki.py is not used by anyone except Makefile, so it is OK to make changes directly into pki.py gen_priv_key and gen_public_key methods.

IIUC, only different is in terms of a few flags right?

[Benouare]

I am trying to understand why can't we modify existing pki.py methods to make them compatible with FF/Chrome.

I didnt touch the existing methods to don't brake the app, but clearly, if we can modify methods and not creating new one, it's the best.

As of today, pki.py is not used by anyone except Makefile, so it is OK to make changes directly into pki.py gen_priv_key and gen_public_key methods.

Get it, I will do that.

IIUC, only different is in terms of a few flags right?

Exactly. The main thing is to sha256, and to add the SAN flag/propertie.

[Benouare]

FYI, if i add a

os.remove(temp_cert)

The e2e test will fail : it looks like the temp_cert is already deleted, but I dont know by who.
It looks like the OS is cleaning the temp dir automatically, so I propose to dont put the os.remove.
I let u think about to drop it in other part of the code and let the OS manage those temp files.

[abhinavsingh]

Exactly. SAN is just needed during certificate signing, so my suggestion is to only change that part of the code. So there are 2 steps happening during interception:

1. Generate a CSR for domain
2. Sign CSR with CA

To generate a CSR we also need domain private key. This is what current code does, it generate an interim key (never saved on disk), pipes the output into CSR signing command.

We'll have to add a similar method where CSR can be generated without having to persist private key (e.g. gen_priv_key, then remove_passphrase, then gen_csr). You can also remove the need of remove_passphrase by not encrypting the private key in first place (i.e. don't use a password while generating one, this will require changes to gen_priv_key flags as it by default only generated encrypted private key). Note that, CSR generation is a one time step and valid till 365 days (current default sign validity).

Also, utilities in pki.py are not in anyway tied with its user. Idea is to simply have all PKI utilities at a central place for re-use. So, I think terms like "web_cert" shouldn't land in pki.py, just continue using standard terminology like private / public keys and signed certificates (for method names).

Hence we must also preserve DEFAULT_CONFIG for use when needed. We can make ways to take input custom config (e.g. SAN during signing), overriding default config.

I hope this background helps you refactor better :)

[abhinavsingh]

@Benouare Thanks for your patience. These have been tough times specially with all the travel involved. Good news, I am back at a stable place and will soon jump into pending PR. Really appreciate it. I hope you are safe and sound in these hard times. Thank you!!!

[abhinavsingh]

This should be resolved with #362 . Apologies for having to close this. I think approach in #362 is simpler (a single method added to server.py) and if it works let's persist with it. Do check it out, thank you for all the help and investigation done on this subject.