Generate trust roots SecCertificate for Transport Services #350

adam-fowler · 2021-03-19T18:08:58Z

This PR is a result of another #321.

In that PR I provided an alternative structure to TLSConfiguration for when connecting with Transport Services.

In this one I construct the NWProtocolTLS.Options from TLSConfiguration. It does mean a little more work for whenever we make a connection, but having spoken to @weissi he doesn't seem to think that is an issue.

Also there is no method to create a SecIdentity at the moment. We need to generate a pkcs#12 from the certificate chain and private key, which can then be used to create the SecIdentity.

This should resolve #292

swift-server-bot · 2021-03-19T18:09:00Z

Can one of the admins verify this patch?

swift-server-bot · 2021-03-19T18:09:00Z

Can one of the admins verify this patch?

weissi

The actual change seems to be lost in reformatting (which also reformats it in the wrong way (eg. moves qualifiers before extension instead of the individual members)). Could we undo everything that's not the actual change you'd like to make?

Sources/AsyncHTTPClient/HTTPClient+HTTPCookie.swift

adam-fowler · 2021-03-21T11:08:00Z

The actual change seems to be lost in reformatting (which also reformats it in the wrong way (eg. moves qualifiers before extension instead of the individual members)). Could we undo everything that's not the actual change you'd like to make?

Sorry I'm running a newer version of swift format and submitted all the other files by mistake.

The public infront of extension is caused by a new rule extensionAccessControl which is defaulted to on by default. I have it disabled in soto because its a pain. You could use it to force the positioning of the public qualifier through. Do you guys intend to upgrade from v0.44.6 at some point?

Lukasa · 2021-03-22T09:21:26Z

@swift-server-bot add to allowlist

Lukasa

Aside from the error handling situation this looks basically good to me.

Lukasa · 2021-03-22T09:26:54Z

Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift

+                        try SecCertificateCreateWithData(nil, Data(certificate.toDERBytes()) as CFData)
+                    }
+                } catch {
+                    // failed to load


I think we need some error handling here.

I stopped catching the error here and set getNWProtocolTLSOptions() to throws. Is that enough?

weissi

happy when @Lukasa is

Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift

weissi · 2021-03-23T12:12:08Z

Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift

                }
+            case .some(.file):
+                preconditionFailure("TLSConfiguration.trustRoots.file is not supported")


same q here. @Lukasa wdyt?

This is a slightly different case. I could probably support this case if I knew what format the file was in.

The file format for trust roots is just bundle-o-pem: a collection of PEM-encoded X509 certs in the same file.

Now loading PEM info NIOSSLCertificate and generating DER for loading into SecCertificateCreateWithData. Possibly not the quickest route.

Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift

adam-fowler · 2021-04-22T14:52:19Z

@Lukasa do we need some more tests for converting between TLSConfiguration and NWProtocolTLS.Options?

adam-fowler · 2021-05-07T10:12:07Z

@Lukasa @weissi Anything left to do here?

Lukasa · 2021-05-11T14:34:21Z

Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift

-                guard case .default = trustRoots else {
-                    preconditionFailure("TLSConfiguration.trustRoots != .default is not supported")
+            var secTrustRoots: [SecCertificate]?
+            switch trustRoots {


A note for the future, we've also added additionalTrustRoots which allows you to do default + others. No need to block this PR on that, but worth noting that it's there.

weissi

Thank you, this looks good to me too! I left one comment (https://github.com/swift-server/async-http-client/pull/350/files#r631742891) which'd be nice to address. Just to be 100% sure we're on the right queue.

weissi

After chatting to @adam-fowler: we're not calling SecEvaluateTrust on the right queue at the moment.

…callstack

Support TLSConfiguration.trustRoots(.file) when converting to a `NWProtocolTLS.Options`

adam-fowler · 2021-05-13T12:14:27Z

After chatting to @adam-fowler: we're not calling SecEvaluateTrust on the right queue at the moment.

And then after reviewing code I realised we were.

Although we did come across another issue where a file load could happen on the EventLoop. To avoid this I have moved the whole NWProtocolTLS.Options generation to the DispatchQueue.

weissi · 2021-05-13T12:16:00Z

Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift

@@ -109,6 +126,11 @@
                preconditionFailure("TLSConfiguration.keyLogCallback is not supported. \(useMTELGExplainer)")
            }

+            // the certificate chain
+            if self.certificateChain.count > 0 {
+                preconditionFailure("TLSConfiguration.certificateChain is not supported")


Suggested change

preconditionFailure("TLSConfiguration.certificateChain is not supported")

preconditionFailure("TLSConfiguration.certificateChain is not supported. \(useMTELGExplainer)")

weissi

Awesome, thank you!

weissi requested changes Mar 20, 2021

View reviewed changes

Sources/AsyncHTTPClient/HTTPClient+HTTPCookie.swift Outdated Show resolved Hide resolved

adam-fowler force-pushed the ts-tls-config2 branch from 7205d36 to 75d8d1c Compare March 21, 2021 10:57

Lukasa requested changes Mar 22, 2021

View reviewed changes

weissi reviewed Mar 23, 2021

View reviewed changes

adam-fowler force-pushed the ts-tls-config2 branch 2 times, most recently from 7b7857b to 7a6826b Compare April 22, 2021 14:42

adam-fowler requested a review from Lukasa April 22, 2021 14:51

adam-fowler requested a review from weissi April 22, 2021 14:52

Lukasa approved these changes May 11, 2021

View reviewed changes

weissi approved these changes May 13, 2021

View reviewed changes

weissi requested changes May 13, 2021

View reviewed changes

adam-fowler added 6 commits May 13, 2021 12:48

Generate trust roots SecCertificate for Transport Services

7fe465e

swift format

c6f048d

Allow errors thrown by SecCertificateCreateWithData to passed up the …

af884fb

…callstack

Support TLSConfiguration.trustRoots(.file)...

ad02945

Support TLSConfiguration.trustRoots(.file) when converting to a `NWProtocolTLS.Options`

Linux tests

869ec00

Create NWProtocolTLS.Options on DispatchQueue

29ea5c3

adam-fowler force-pushed the ts-tls-config2 branch from 28c56b2 to 29ea5c3 Compare May 13, 2021 12:10

adam-fowler mentioned this pull request May 13, 2021

Add NIOTransportServices TLS Configuration options #321

Closed

weissi reviewed May 13, 2021

View reviewed changes

Added dispatchPreCondition

508233a

weissi approved these changes May 13, 2021

View reviewed changes

weissi merged commit 9cdf8a0 into swift-server:main May 13, 2021

weissi added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label May 13, 2021

adam-fowler deleted the ts-tls-config2 branch May 13, 2021 13:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate trust roots SecCertificate for Transport Services #350

Generate trust roots SecCertificate for Transport Services #350

adam-fowler commented Mar 19, 2021 •

edited

swift-server-bot commented Mar 19, 2021

swift-server-bot commented Mar 19, 2021

weissi left a comment

adam-fowler commented Mar 21, 2021

Lukasa commented Mar 22, 2021

Lukasa left a comment

Lukasa Mar 22, 2021

adam-fowler Mar 22, 2021

weissi left a comment

weissi Mar 23, 2021

adam-fowler Mar 23, 2021

Lukasa Apr 22, 2021

adam-fowler Apr 22, 2021

adam-fowler commented Apr 22, 2021

adam-fowler commented May 7, 2021

Lukasa May 11, 2021

weissi left a comment

weissi left a comment

adam-fowler commented May 13, 2021

weissi May 13, 2021

weissi left a comment

	preconditionFailure("TLSConfiguration.certificateChain is not supported")
	preconditionFailure("TLSConfiguration.certificateChain is not supported. \(useMTELGExplainer)")

Generate trust roots SecCertificate for Transport Services #350

Generate trust roots SecCertificate for Transport Services #350

Conversation

adam-fowler commented Mar 19, 2021 • edited

swift-server-bot commented Mar 19, 2021

swift-server-bot commented Mar 19, 2021

weissi left a comment

Choose a reason for hiding this comment

adam-fowler commented Mar 21, 2021

Lukasa commented Mar 22, 2021

Lukasa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-fowler commented Apr 22, 2021

adam-fowler commented May 7, 2021

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

adam-fowler commented May 13, 2021

Choose a reason for hiding this comment

weissi left a comment

Choose a reason for hiding this comment

adam-fowler commented Mar 19, 2021 •

edited