-
Notifications
You must be signed in to change notification settings - Fork 22
Load multiple certificates from a single trust root file #115
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
Conversation
Motivation: When passing a PEM file with multiple certificates as trust roots only, the first one would be loaded. This is unintuitive. Modifications: Instead of creating a NIOSSLCertificate directly, use a function that loads multiple certificates from a PEM file. Add a test case that creates a certificate chain in a temporary directory to test the expected behavior. Result: Multiple certificates can be loaded from a single file.
| try NIOSSLCertificate( | ||
| bytes: bytes, | ||
| format: NIOSSLSerializationFormats(serializationFormat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably use fromPEMBytes here as well
| file: path, | ||
| format: NIOSSLSerializationFormats(serializationFormat) | ||
| ) | ||
| switch NIOSSLSerializationFormats(serializationFormat) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the indirection here? Can we just switch over serializationFormat.wrapped?
| certificate: String, | ||
| key: String, | ||
| trustRoots: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add Path to the end of each of these?
| certificate: String, | ||
| key: String, | ||
| trustRoots: String |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, can you add Path to the end of each of these?
| case server | ||
| } | ||
|
|
||
| /// Writing the files to disk returns a dictionary with these keys to access the file locations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than a dictionary can we just have a struct with a property for each of these? We can avoid the ! when accessing them each time then.
| } | ||
|
|
||
| /// The domains names for the leaf certificates | ||
| let serverName = "my.server" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This matters much less because it's in tests, but it's worth mentioning anyway. In structs, let is very rarely the right choice (vs var). In this instance it should really be a static so that it's not stored inline in the struct.
In general though, if this was passed in by the caller in the init then it should still be a var, because there's nothing stopping the caller from creating a new instance with a different value:
struct CertificateChain {
let serverName: String
init(serverName: String) { self.serverName = serverName }
}
let chain = CertificateChain(serverName: "foo")
chain.serverName = "bar" // not allowed; chain isn't mutable
var chain2 = chain
chain2.serverName = "bar" // Agh, not allowed!
chain2 = CertificateChain(serverName: "bar") // allowed, but annoyingIf it were a var:
struct CertificateChain {
var serverName: String
init(serverName: String) { self.serverName = serverName }
}
let chain = CertificateChain(serverName: "foo")
chain.serverName = "bar" // not allowed; chain isn't mutable
var chain2 = chain
chain2.serverName = "bar" // allowed, great!| /// - Parameters: | ||
| /// - fileTag: A prefix added to all certificates files | ||
| /// - Returns: A dictionary storing mapping `CertificateChain.Files` to the respective file names | ||
| public func writeToTemp(fileTag: String) throws -> [Files: String] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you put #function it will default to the calling function.
| public func writeToTemp(fileTag: String) throws -> [Files: String] { | |
| public func writeToTemp(fileTag: String = #function) throws -> [Files: String] { |
| let fm = FileManager.default | ||
| let directory = fm.temporaryDirectory | ||
|
|
||
| var fileNames = [Files: String]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just use a nominal type here
This reverts commit 7a4dd68.
glbrntt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
### What changes were proposed in this pull request? This PR aims to upgrade `gRPC Swift NIO Transport` to 2.1.0. ### Why are the changes needed? To bring the latest improvements. - https://github.com/grpc/grpc-swift-nio-transport/releases/tag/2.1.0 - grpc/grpc-swift-nio-transport#122 - grpc/grpc-swift-nio-transport#120 - grpc/grpc-swift-nio-transport#115 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #218 from dongjoon-hyun/SPARK-53371. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>
### What changes were proposed in this pull request? This PR aims to upgrade `gRPC Swift NIO Transport` to 2.1.0. ### Why are the changes needed? To bring the latest improvements. - https://github.com/grpc/grpc-swift-nio-transport/releases/tag/2.1.0 - grpc/grpc-swift-nio-transport#122 - grpc/grpc-swift-nio-transport#120 - grpc/grpc-swift-nio-transport#115 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Pass the CIs. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #218 from dongjoon-hyun/SPARK-53371. Authored-by: Dongjoon Hyun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit a94af2e) Signed-off-by: Dongjoon Hyun <[email protected]>
Motivation
When passing a PEM file with multiple certificates as trust roots, only the first one is loaded.
Modifications
Instead of creating a NIOSSLCertificate directly, use a function that loads multiple certificates from a PEM file. Add a test case that creates a certificate chain in a temporary directory to verify the expected behavior.
Result
Multiple certificates can be loaded from a single PEM file.