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

Enable TLS Configuration with in-memory PEM strings #2

Closed
9 tasks done
CMCDragonkai opened this issue Apr 11, 2023 · 13 comments
Closed
9 tasks done

Enable TLS Configuration with in-memory PEM strings #2

CMCDragonkai opened this issue Apr 11, 2023 · 13 comments
Assignees
Labels
development Standard development r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices

Comments

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Apr 11, 2023

Specification

In QUIC our current system requires passing a file path into QUICConfig.

{
  certChainFromPemFile: string | undefined;
  privKeyFromPemFile: string | undefined;
}

These 2 properties are required on the server side, while they are optional on the client side. PK will of course use these 2 on both client and server.

These 2 properties are later used by config.buildQuicheConfig:

  if (config.certChainFromPemFile != null) {
    quicheConfig.loadCertChainFromPemFile(config.certChainFromPemFile);
  }
  if (config.privKeyFromPemFile != null) {
    quicheConfig.loadPrivKeyFromPemFile(config.privKeyFromPemFile);
  }

It's better to be able to pass PEM strings, then require the QUIC client and QUIC server to read certificates and keys from the local filesystem. It's more secure as we don't leave any secrets on the local filesystem.

In order to do this, we need to add a new factory method to the config.rs.

#[napi(factory)]
pub fn with_boring_ssl_ctx(version: i64, certPEM: String, keyPEM: String) -> Result<Self> {
  // ... figure out this ...
}

The body of this function will need to take the PEM strings bridged by napi-rs, and then use the boring package to turn them into the x509 objects.

Subsequently use boring::ssl::SslContext::builder to create a SSL context builder, which then takes those objects and builds the SSL context.

Finally it should be possible to use quiche::Config::with_boring_ssl_ctx to finally build a config object with string cert and key.

Currently in PK, we have the types:

CertificatePEMChain
PrivateKeyPEM

Both are string types. These will be used when we want to load certs and key into the QUIC connection.

Tests should test the usage of this on both client and server, with the verifyPeer option set to true.

Tests should ensure that certificate chains are also accepted. The cert chain PEM doesn't just mean 1 certificate, it could have a whole chain of certificates.

An additional problem is dealing with certificate and key rotation. I'll address this in a separate issue.

Additional context

Tasks

  1. Bring in the boring package into Cargo.toml.
  2. Update the shell.nix with rustPlatform.bindgenHook so that boring can actually compile
  3. Update the config.rs with the with_boring_ssl_ctx method. Use chat gpt and github copilot to help you find out what kind of code to write.
  4. Compile it with npm run napi-build.
  5. Expose the method in src/native/types.ts
  6. Change the buildQuicheConfig to take the TLS cert pem and key pem strings.
  7. Eliminate the file path options, we won't even bother implementing these.
  8. Change tests to stop using the localhost.key and localhost.crt, but instead synthetically generate these 2 in-memory now. You can use peculiar's x509 library as a devDependency and follow the PK's src/keys/utils/x509.ts to understand how to generate a relevant certificate chain for testing purposes.
  9. You can sanity check by reading the localhost.crt and localhost.key first as strings. But it's better we can use fastcheck to generate different kinds of certs for testing.
@CMCDragonkai
Copy link
Member Author

One additional thing, I forgot about the load_verify_locations_from_file.

This can also be turned into an in-memory thing. Which should then be part of the above with_boring_ssl_ctx.

That would extend it to something like:

with_boring_ssl_ctx(version: i64, certPEM: String, keyPEM: String, caPEM: String) -> Result<Self> {
  // ...
}

But one thing though... it may be a good idea to make it optional. With Option<String>.

That way if it's not provided, then quiche will do the default. I'm not entirely sure if the default is just that it does nothing.

Tests should indicate whether it does actually automatically read the OS's cert store. I suspect not, and that it would be an application concern.

This feeds into the rotation issue #3, since it would also be possible to rotate the caPEM.

However application-wise we wouldn't be using caPEM anyway because we have to disable the default verifyPeer, and instead use our own custom verification logic that we originally have in PK's network/utils.

@tegefaulkes
Copy link
Contributor

I have it working now, at least, it compiles and runs in the tests. The QUICClient tests are timing out. I don't know if this is a problem with the config.

@CMCDragonkai
Copy link
Member Author

Were they timing out before the change?

I was working on artificially testing if the server did not exist. So timing out was actually what I wanted.

If you want it to not timeout. You should renable the server. Remember I was experimenting with the dialling phase so I didn't want a server to exist.

I don't think I uncommented out my experiments. Check the code of the quic client tests.

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Apr 13, 2023

I can bring in a bunch of stuff from Polykey for generating the keys, keypairs and pems. But a lot of type definitions come along with that. Do I bring over the types as well or simplify them down to the primitives they're derived from?

I'll also bring in the fast check stuff.

@tegefaulkes
Copy link
Contributor

Trying to generate the TLS configs is blowing out the dependencies and scope of this issue. I think I should branch that part off and address it in a separate issue. Hard coding the pems for tests is functional for now.

@CMCDragonkai
Copy link
Member Author

You only need 1 thing to generate TLS certs. The x509 library.

You can generate keys using regular Node's webcrypto.

@CMCDragonkai
Copy link
Member Author

Node actually has a x509 built in too, but I reckon it's better to just use the x509 library directly since we already have examples of how to do that in PK.

And no you don't need to bring all the types, just do the most simplest possible certificate and certificate chain tests. You should test that a cert chain also works too.

@CMCDragonkai
Copy link
Member Author

Would be good to however generate ed25519 keys for the certs, because those are the keys we use for our x509 certs in PK. The latest version of node does support ed25519 keys.

@CMCDragonkai
Copy link
Member Author

Due to this PeculiarVentures/webcrypto#55, see src/key/utils/webcrypto, it may be necessary, I forgot.

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Apr 13, 2023

I went back to a previous commit to see if the QUICClient tests were timing out then as well. So I can't really tell if the config changes are working beyond the fact that it's not actively throwing when used. I think that covers this issue. I'll make a new issue for generating the TLS certs with fast-check and continue with that.

I'll close this issue with a commit message, I don't think there is a need for a PR.

@tegefaulkes
Copy link
Contributor

This issue will auto close when the commit hits the main branch.

Do you want to look over the changes?

@CMCDragonkai
Copy link
Member Author

I went back to a previous commit to see if the QUICClient tests were timing out then as well. So I can't really tell if the config changes are working beyond the fact that it's not actively throwing when used. I think that covers this issue. I'll make a new issue for generating the TLS certs with fast-check and continue with that.

I'll close this issue with a commit message, I don't think there is a need for a PR.

They were timing out because there's no server. Just re-enable the server.

@CMCDragonkai
Copy link
Member Author

CMCDragonkai commented Apr 13, 2023

Changing the test to:

  test('times out when there is no server', async () => {
    // QUICClient repeatedly dials until the connection timesout
    await expect(QUICClient.createQUICClient({
      host: '127.0.0.1' as Host,
      port: 55555 as Port,
      localHost: '127.0.0.1' as Host,
      crypto,
      logger: logger.getChild(QUICClient.name),
      config: {
        maxIdleTimeout: 1000,
      }
    })).rejects.toThrow(errors.ErrorQUICConnectionTimeout);
  });

Then re-running everything works.

 PASS  tests/QUICClient.test.ts
  QUICClient
    ✓ times out when there is no server (3017 ms)
    dual stack client
      ✓ to ipv4 server succeeds (163 ms)
      ✓ to ipv6 server succeeds (83 ms)
      ✓ to dual stack server succeeds (83 ms)

Test Suites: 1 passed, 1 total
Tests:       4 passed, 4 total
Snapshots:   0 total
Time:        3.916 s, estimated 4 s
Ran all test suites matching /.\/tests\/QUICClient.test.ts/i.
GLOBAL TEARDOWN

tegefaulkes added a commit that referenced this issue Apr 13, 2023
CMCDragonkai added a commit that referenced this issue May 17, 2023
CMCDragonkai pushed a commit that referenced this issue May 17, 2023
@CMCDragonkai CMCDragonkai added the r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices label Jul 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices
Development

No branches or pull requests

2 participants