Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to constrain root CA to permitted domains #175

Merged
merged 7 commits into from
Jul 10, 2024

Conversation

lipsumar
Copy link
Contributor

@lipsumar lipsumar commented Jul 5, 2024

We have a use-case where we would like to trust the Root CA generated by mockttp, in order to use mockttp as a dev tool. However, as mentioned in this page, trusting this certificate raises security concerns.

In order to mitigate the security risk linked to trusting the CA, this PR adds an option contrainToDomains to the generateCACertificate() function. Using this option will add a nameConstraints extension, severely limiting the scope of the CA. The CA will then only be able to correctly sign certificates for the provided domains.

The code is based on this forge issue and appears to work as intended from my tests. I have only included support for a whitelist as it seems to make most sense from a security standpoint, but happy to add support for a blacklist as well. The generateNameConstraints will be modified in consequence.

Should you be receptive to this change, I would really appreciate some guidance on testing this. My understanding is tests should be added in test/ca.spec.ts. How would you suggest to go about testing this ? My initial approach would be to generate a certificate inside/outside the nameConstraint and expect fetch to accept/reject a request to a server using these certificates.

Copy link
Member

@pimterry pimterry left a comment

Choose a reason for hiding this comment

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

A few comments on the details I'd like to tweak, but overall this looks good to me (as far as I can tell without testing in depth) and I definitely see how it could be useful. Great idea! Thanks for the PR 👍

I would really appreciate some guidance on testing this. My understanding is tests should be added in test/ca.spec.ts. How would you suggest to go about testing this ? My initial approach would be to generate a certificate inside/outside the nameConstraint and expect fetch to accept/reject a request to a server using these certificates.

That approach makes sense to me. In ca.spec.ts there's a simple test that checks that a generated cert is accepted by fetch (here). I'd suggest creating a few variants of that, within the CA certificate generation section below, which covering the positive & negative cases for CA certs. If you test with *.localhost domains (or *.*.localhost if you really want) then you'll have domains that always resolve to the local machine (so always reach the test server) but let you check subdomains etc.

It would also be good to add a lintcert test for a CA certificate including these name extensions (lintcert is very good for checking spec validity, and catching issues that can cause unexpected client cert rejections later) - there's plenty other examples in that file for those.

@lipsumar
Copy link
Contributor Author

lipsumar commented Jul 8, 2024

I have some questions regarding the tests (ca.spec.ts):

  • the script test:node specifies NODE_EXTRA_CA_CERTS as CLI argument. For the tests I'm adding, we would add one more custom CA that node should be trusting. Should I add a constrained CA cert to test/fixtures/test-ca.pem ? I don't find an easy way to add an extra CA cert at runtime, especially if we need to support node under and above v18 (globalAgent & undici)
  • about using subdomains of localhost, I find that for example foo.localhost throws ENOTFOUND (as I would have expected). Is there something I'm missing here ?

EDIT:

  • adding the constrained cert to test/fixtures/test-ca.pem works, but we'll also need a separate pem file with only the constrained cert (to create the CA with, so it's properly constrained. Creating it with both certs still allows it to sign any domain)
  • I'm currently using a hosts file to point hello.localhost and hello.otherhost to 127.0.0.1
describe('constrained CA', () => {
    const constrainedCaKey = fs.readFile(path.join(__dirname, 'fixtures', 'constrained-ca.key'), 'utf8');
    const constrainedCaCert = fs.readFile(path.join(__dirname, 'fixtures', 'constrained-ca.pem'), 'utf8');

    it("can generate a valid certificate for a domain included in a constrained CA", async () => {
        const ca = new CA({ key: await constrainedCaKey, cert: await constrainedCaCert });
        const { cert, key } = ca.generateCertificate('hello.localhost');

        server = https.createServer({ cert, key }, (req: any, res: any) => {
            res.writeHead(200);
            res.end('signed response!');
        });
        await new Promise<void>((resolve) => server.listen(4430, resolve));
        
        await expect(fetch('https://hello.localhost:4430')).to.have.responseText('signed response!');
    });

    it("can not generate a valid certificate for a domain not included in a constrained CA", async () => {
        const ca = new CA({ key: await constrainedCaKey, cert: await constrainedCaCert });
        const { cert, key } = ca.generateCertificate("hello.otherhost");
    
        server = https.createServer({ cert, key }, (req: any, res: any) => {
            res.writeHead(200);
            res.end("signed response!");
        });
        await new Promise<void>((resolve) => server.listen(4430, resolve));
    
        await expect(fetch("https://hello.otherhost:4430")).rejectedWith(
            "permitted subtree violation"
        );
    });
});

@pimterry
Copy link
Member

pimterry commented Jul 8, 2024

For the tests I'm adding, we would add one more custom CA that node should be trusting. Should I add a constrained CA cert to test/fixtures/test-ca.pem?

I don't think you want a pre-built CA cert file for these tests at all. Instead, I would just generate a fresh CA every time within the tests somewhere (it's a bit clearer & simpler to do so, and avoids depending on fixed fixtures that'll need updating later e.g. when the cert expires or if we change how the CA is generated).

NODE_EXTRA_CA_CERTS is useful to change the default list of CA certs that are trusted, but at runtime you can still manually trust whatever certificates you like without that. Unfortunately I'm not sure that fetch supports this, which makes things slightly more awkward, but if you use Node's built-in https.request(...) method instead then you just need to pass a ca: [pemData] option to specify which CA to trust.

about using subdomains of localhost, I find that for example foo.localhost throws ENOTFOUND (as I would have expected). Is there something I'm missing here ?

That's very interesting! Turns out this commonly resolves locally, and it's reserved as a gtld specifically for that purpose (so it'll never resolve remotely), but it's not actually built-in as a reliable resolution everywhere. That's a bit inconvenient, since we'd quite like some reliable subdomains we can use that'll always map to localhost.

You should be able to work around this by passing a lookup option to the https.request method, which will let you just map all hostnames to localhost manually instead. I think it should be something approximately like:

lookup: (_name, _opts, cb) => cb(null, [{ address: '127.0.0.1', family: 4 }])

@lipsumar
Copy link
Contributor Author

lipsumar commented Jul 9, 2024

I don't think you want a pre-built CA cert file for these tests at all.

100% agree, thanks for the guidance on specifying a CA at runtime, works like a charm!

Tests seem to pass for node, but they now seem stuck at webpack. I didn't expect certificate creation to happen in the browser though, any pointers to solve this ?

@pimterry pimterry merged commit fc2f82e into httptoolkit:main Jul 10, 2024
9 checks passed
@pimterry
Copy link
Member

Great stuff, thanks! Merged 👍

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.

2 participants