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

Support loading trust root CAs on Linux #136

Merged
merged 16 commits into from
Oct 18, 2023

Conversation

dnadoba
Copy link
Member

@dnadoba dnadoba commented Sep 22, 2023

Motivation

We want to eventually replace the BoringSSL X.509 layer in swift-nio-ssl with swift-certificates. One missing feature in swift-certificates is the loading of trusted root CAs from the system.

Changes

  • add CertificateStore.systemTrustRoots that lazily loads the trusted root certificates from disk.
  • add CertificateStore.insert(_:) and friends to add additional trusted root CAs to the CertificateStore.systemTrustRoots CertificateStore
  • add LockedValueBox, Promise and Future to the internal API to support lazy loading of the trust roots
  • log diagnostics if loading of the system trust roots fails during verification

Result

swift-certificates can efficiently load the installed trusted root CAs on Linux.

@dnadoba dnadoba added the semver/minor Adds new public API. label Sep 22, 2023
@dnadoba
Copy link
Member Author

dnadoba commented Sep 22, 2023

5.9 (nightly) and main nightly crashes because of this compiler bug: swiftlang/swift#68708

Sources/X509/LockedValueBox.swift Outdated Show resolved Hide resolved
Sources/X509/Verifier/CertificateStore.swift Outdated Show resolved Hide resolved
Sources/X509/Verifier/CertificateStore.swift Outdated Show resolved Hide resolved
Sources/X509/Verifier/CertificateStore.swift Outdated Show resolved Hide resolved
}

@usableFromInline
var _certificates: _TinyArray<Element>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there ever more than two elements here? If not, can we use an enum instead of a TinyArray?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought of allowing to combing certificate stores which would in theory allow up to 3 elements i.e. [.customCertificates(...), .trustRoots, .customCertificates(...)]. The current public API doesn't allow that though and I don't have a concrete use case in mind for that. Do you think something like that is useful or not? If not, we can go with a tuple. I think it makes the implementation a bit more complicated as we can't iterate over tuples but hopefully it will not be too bad.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think sticking with the tuple is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have now implemented _TinyArray2 which store up to two elements inline.

Sources/X509/Verifier/TrustRootLoading.swift Outdated Show resolved Hide resolved
Sources/X509/Verifier/TrustRootLoading.swift Outdated Show resolved Hide resolved
Sources/X509/Verifier/TrustRootLoading.swift Outdated Show resolved Hide resolved
Sources/X509/Verifier/TrustRootLoading.swift Outdated Show resolved Hide resolved
Tests/X509Tests/CertificateStore.swift Outdated Show resolved Hide resolved
@tayloraswift
Copy link

5.9 (nightly) and main nightly crashes because of this compiler bug: apple/swift#68708

this package seems to be experiencing a second, seemingly different compiler crash as well: swiftlang/swift#68767 , which affects documentation generation.

@dnadoba
Copy link
Member Author

dnadoba commented Sep 26, 2023

@tayloraswift thanks for reporting! The crash you have discovered is even on 5.9-release, mine is only on 5.9 nightly. I have worked around this issue for now. Not sure if we can workaround the other crash.

@dnadoba
Copy link
Member Author

dnadoba commented Sep 27, 2023

API break is a false positive:

10:02:32   💔 API breakage: constructor CertificateStore.init(_:) has generic signature change from <Certificates where Certificates : Swift.Sequence, Certificates.Element == X509.Certificate> to 

We have changed the signature but only from:

public init<Certificates: Sequence>(_ certificates: Certificates) where Certificates.Element == Certificate

to

public init(_ certificates: some Sequence<Certificate>)

which is the same thing, just with a different spelling.

Bug reported: swiftlang/swift#68797

}

deinit {
switch self.state.unsafeUnlockedValue {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this optimization is appropriate: we can just take the lock, it'll be uncontended.

@usableFromInline
var _certificates: [DistinguishedName: [Certificate]]
var additionTrustRoots: [DistinguishedName: [Certificate]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: "additional"

var systemTrustRoots: [DistinguishedName: [Certificate]]

@usableFromInline
var additionTrustRoots: [DistinguishedName: [Certificate]]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: additional.

}()

static let cachedSystemTrustRootsFuture: Future<[DistinguishedName: [Certificate]], any Error> =
DispatchQueue.global(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer to create a serial dispatch queue for this than directly dispatching to a global queue.

@Lukasa
Copy link
Collaborator

Lukasa commented Oct 18, 2023

Merging over API breakage checker, which is wrong here.

@Lukasa Lukasa merged commit 3c33085 into apple:main Oct 18, 2023
6 of 7 checks passed
@dnadoba dnadoba deleted the dn-system-trust-roots-linux branch October 18, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants