Allow passing in trustAnchors to newTLSClientAsyncStream

[iffy]

I'd like to be able to use my own certs, including self-signed certs in some TLS connections. This change lets me provide my own trust anchors.

[Menduist]

Be careful, you pushed tests/testasyncstream binary file

[iffy]

Be careful, you pushed tests/testasyncstream binary file

Oops... in all my nim projects I gitignore these. I'll back it out.

[iffy]

Alright, I've pushed a new version that allows for passing an optional seq instead of an array.

I contemplated not using Option and instead using the Mozilla TAs if len(seq) == 0 but thought that could lead to users intending to use their own TAs and accidentally using Mozillas. But I understand if you don't want to introduce Options here.

Any other thoughts? Thank you for reviewing the mess I've put up so far :)

[cheatfate]

1. Option[T] will not going to be accepted.
2. Multiple waitFor (use await) in test is not going to be accepted.
3. openArray[T] should be used instead of seq[T] version, for many reasons. Trusted anchors database should be non-mutable data storage. seq[T] is mutable. So with openArray[T] we assume that certificates will be stored in process data section or present for whole process time. Nim will not allow us to create this specific limitations, but i prefer openArray here. Maybe there should be some comments left about how list of trusted anchors should be stored.
4. There should be assertion check for an empty array.

[iffy]

1. Option[T] will not going to be accepted.

👍

2. Multiple `waitFor` (use `await`) in test is not going to be accepted.

👍

3. `openArray[T]` should be used instead of `seq[T]` version, for many reasons. Trusted anchors database should be non-mutable data storage. `seq[T]` is mutable. So with `openArray[T]` we assume that certificates will be stored in process data section or present for whole process time. Nim will not allow us to create this specific limitations, but i prefer `openArray` here. Maybe there should be some comments left about how list of trusted anchors should be stored.

• Both seq[T] and openArray[T] are mutable, though the array length is not. A caller could change either after passing them in.
• Aren't seq passed by copy? So in this code each TlsAsyncStream gets a copy of the TAs, right? Or is it only a copy of the reference to the TA?

• If a comment is the only thing standing between memory safety and not, I'm not sure how an array is better than a seq. I imagine there is a way to make it surely memory safe by forcing callers to construct and pass in a struct/object that owns a copy of the TA data (but doesn't make a copy per TlsAsyncStream). I may attempt this method, because I'd like memory safety to be more foolproof.

  4. There should be assertion check for an empty array. 👍

[cheatfate]

TlsAsyncStream did not get copy of the TAs, bearssl using reference, so if seq will be garbage collected you will get SIGSEGV on certificate verification. My idea is to have just single copy of TA for whole process (there is no reason to have multiple arrays with TAs around). And this copy must not be mutable (because of security reasons). So maybe static could help us, but this will prohibit future work on cacert.pem parsing...

[iffy]

there is no reason to have multiple arrays with TAs around

@cheatfate My purpose in submitting this PR is to allow users of my program to set their TA at runtime, so static wouldn't work, right? I'm aware that the code currently doesn't copy TAs (it just uses the const Mozilla ones built in to nim-bearssl).

What about something like this?

type
  TrustAnchorStore = ref object
    tas: seq[X509TrustAnchor]

proc newTrustAnchorStore(anchors: openArray[X509TrustAnchor]): TrustAnchorStore
  new(result)
  for anchor in anchors:
    result.tas.add(copy(anchor)) # I'm not sure what the actual `copy` proc is, but this makes a copy

type
  TLSAsyncStream = ...
    ...
    trustAnchors*: TrustAnchorStore

proc newTLSClientAsyncStream*(
  ...
  trustAnchors = TrustAnchorStore | static[openArray[X509TrustAnchor]]
  ): TLSAsyncStream =
  ...

Callers of newTLSClientAsyncStream could still pass in a const array of TAs or pass nothing at all and get the Mozilla TAs. Or if they want runtime TAs, they could construct a TrustAnchorStore which will contain a copy of their TAs. They could reuse the same TrustAnchorStore for different calls, and there would be no danger of garbage collection because the TLSAsyncStream would keep a reference to the TrustAnchorStore.

Users might accidentally make too many TrustAnchorStores if they don't read the comments about reusing the store, but they can't accidentally get a segfault.

[cheatfate]

I dont want to allow people to change TAs at runtime at any cost but i wish to allow people to specify custom TAs.

1. We trying to keep version bundled in nim-bearssl up to date.
2. If somebody needs self-signed certificates, its possible to be done by using TLSFlags.
3. We going to work on cacert.pem parser and facility which will try to find this file in different OS.

What the real purpose of your PR, are you trying to alter official TAs list to add some "custom/malicious" authorities to be able to use this authorities to create new certificates?

[iffy]

What the real purpose of your PR, are you trying to alter official TAs list to add some "custom/malicious" authorities to be able to use this authorities to create new certificates?

Nothing malicious. I'm not trying to alter the official Mozilla TA list -- nothing in my PR does that. If my change is accepted, people using chronos will continue to use the Mozilla TAs unless they choose to supply a different TA list.

I'm making an application that can start websockets servers and clients (via nim-websock). Some of the users will want to use their own self-signed certificates to secure the connection between their two devices. The application will generate a self-signed cert for the server at runtime. They will then provide their client with the server's public key. The client will use that public key as the only TA when connecting to their server. That's it.

This is a use case described as "Non-CA trust anchors" in the 4th bullet point of these BearSSL docs: https://www.bearssl.org/x509.html#the-minimal-engine

Non-CA trust anchors implement direct trust: if the EE certificate matches a non-CA trust anchor (subjectDN identical to the anchor name, and same public key), then validation succeeds. The EE certificate itself is still fully decoded to allow for name extraction and matching, and key usage gathering.

The test case here shows pretty much exactly what I'm hoping to do, which is to make a TLS client that trusts only one TLS server: https://github.com/status-im/nim-chronos/pull/355/files#diff-7c38a4d2f5084806ec39ffde17cc1f0bccb0fd4802abecc5f9c2f8511208a71cR983

[iffy]

2. If somebody needs self-signed certificates, its possible to be done by using TLSFlags.

If there's a way to do this with TLSFlags that would be great! But I'm not seeing a way to specify a specific public key to trust here: https://github.com/status-im/nim-chronos/blob/master/chronos/streams/tlsstream.nim#L26 I only see options for not verifying certs.

[cheatfate]

Exactly, so you creating self-signed certificate, start your server with this certificate and use client with TLSFlags.NoVerifyHost to connect to your server with self-signed certificate. In such case client will not verify certificate using TAs.

[iffy]

I don't want to disable verification. I want the clients to verify that the server they are connecting to is the one they expect and the only server they trust.

[iffy]

I've pushed a new version that hopefully addresses the items from before:

1. Preserves existing behavior regarding using the Mozilla TAs -- if people don't pass in their own trustAnchors the MozillaTrustAnchors are used.
2. No use of Option
3. Using await instead of waitFor in the test
4. Asserts the size of TAs is > 0
5. Keeps a reference to custom TAs to prevent them from being garbage collected, but also allows reusing a TrustAnchorStore among several clients.

[cheatfate]

Looks like we unable to achieve consensus here, you proposing to include list of TAs with every TLSAsyncStream instance, so every object will occupy +200KB of memory without any reason, sorry but i can't accept it.

I don't want to disable verification. I want the clients to verify that the server they are connecting to is the one they expect and the only server they trust.

Clients are unable to verify that server they are connecting is the one they expect, it can happens only when server's certificate can be verified with some well-known list of TAs (like Mozilla's one). If you want to establish such type of trust you should buy official SSL certificate or get free one from https://letsencrypt.org/. And in such way you dont need to alter Mozilla's list of TAs.

[iffy]

so every object will occupy +200KB of memory without any reason

I don't believe this is true, but please correct my understanding if I'm wrong:

If you call newTLSClientAsyncStream without specifying trustAnchors you will use the MozillaTrustAnchors, not a copy of them, just like the current code does.

If you call newTLSClientAsyncStream with an instance of TrustAnchorStore, because TrustAnchorStore is a ref object and allocated on the heap, it will only store a reference, not a copy. Users who want to use a custom TrustAnchorStore should be advised that they should only make one store and pass the same instance to all newTLSClientAsyncStream calls. (I forgot to add a comment but I can add this).

var mystore = newTrustAnchorStore(...)
var client1 = newTLSClientAsyncStream(..., trustAnchors=mystore)
var client2 = newTLSClientAsyncStream(..., trustAnchors=mystore)

In the above code, client1 and client2 will share the same mystore, not separate copies, correct?

Clients are unable to verify that server they are connecting is the one they expect, it can happens only when server's certificate can be verified with some well-known list of TAs (like Mozilla's one).

This is not true. Neither TLS nor BearSSL require users to use a "well-known list of TAs." The set of TAs a server/client use is up to the server/client. It's convenient that chronos includes Mozilla's TAs for making connections, but it's not a requirement to only use Mozilla's TAs.

In the use-case I'm trying to support, my users aren't hosting things on the public Internet. They're not programmers and they wouldn't have a clue how to use letsencrypt. This is a desktop program that they run on a few computers. I want them to be able to connect their computers together without eavesdropping on their local/business network. It will work like this:

1. On computer 1, they press the "Start server" button.
2. This creates a self-signed TLS certificate.
3. On computer 2, they press the "Start client" button.
4. It asks them to put in the certificate of the server they want to connect to. They copy and paste it from one computer to the other.
5. Computer 2 can then connect securely to computer 1, with full TLS, including certificate validation.

In this scenario, the certs must be generated and anchors must be chosen at runtime. If I were to compile in the trusted anchors and then distribute it to my users, it would not be secure for them because

1. I would have the private keys they were using to secure their connection.
2. Anyone who gets the program could impersonate the server since all copies of the program contain the same keys.

And in such way you dont need to alter Mozilla's list of TAs.

I'm not trying to alter Mozilla's list of TAs. None of the code I've pushed alters Mozilla's list of TAs. I don't know why you keep claiming that.

---

What I'm proposing isn't strange. Many projects allow you to provide custom TAs. For example:

• Python's requests has the verify="/path/to/cert.pem" option
• As already mentioned, BearSSL lets you provide a "Non-CA trust anchor"
• Rust tls lets you specify anchors with OwnedTrustAnchor
• Go tls lets you use a custom root cert

[cheatfate]

Sorry for long reply, but this small reproducible source working for me in 1.6

type
  X509TrustAnchor* = distinct string

  TrustAnchorStore* = ref object
    anchors: seq[X509TrustAnchor]

proc new*(T: typedesc[TrustAnchorStore],
          anchors: openArray[X509TrustAnchor]): TrustAnchorStore =
  var res: seq[X509TrustAnchor]
  for anchor in anchors:
    res.add(anchor)
    doAssert(unsafeAddr(anchor) != unsafeAddr(res[^1]),
             "Anchors should be copied")
  TrustAnchorStore(anchors: res)

when isMainModule:
  let store = TrustAnchorStore.new([X509TrustAnchor("test1"),
                                    X509TrustAnchor("test2"),
                                    X509TrustAnchor("test3"),
                                    X509TrustAnchor("test4")])
  echo string(store.anchors[0])

[cheatfate]

@iffy could you please update PR?

[iffy]

@cheatfate I just pushed the change and the tests now pass. Thank you!