Proposal to fix #362. #363
Conversation
Allows user to setup a channel with a client certificate and key but still use default root certificates.
Sources/SwiftGRPC/Core/Channel.swift
Outdated
| @@ -75,17 +75,18 @@ public class Channel { | |||
| /// Initializes a gRPC channel | |||
| /// | |||
| /// - Parameter address: the address of the server to be called | |||
| /// - Parameter certificates: a PEM representation of certificates to use | |||
| /// - Parameter certificates: a PEM representation of certificates to use. If nil, use defaults. | |||
There was a problem hiding this comment.
nit: "If nil, the default root certificates provided by gRPC are used."
Sources/SwiftGRPC/Core/Channel.swift
Outdated
| /// - Parameter clientCertificates: a PEM representation of the client certificates to use | ||
| /// - Parameter clientKey: a PEM representation of the client key to use | ||
| /// - Parameter arguments: list of channel configuration options | ||
| public init(address: String, certificates: String, clientCertificates: String? = nil, clientKey: String? = nil, arguments: [Argument] = []) { | ||
| public init(address: String, certificates: String?, clientCertificates: String? = nil, clientKey: String? = nil, arguments: [Argument] = []) { |
There was a problem hiding this comment.
certificates: String? = nil, or (if that compiles, please try it and let me know if it doesn't) certificates: String = roots_pem().
There was a problem hiding this comment.
@MrMage
roots_pem() returns an optional, so we have to either force unwrap roots_pem() either leave argument optional: certificates: String? = roots_pem()
I'm not sure which way better. Looking forward to your decision.
There was a problem hiding this comment.
Thanks for the explanation! I think we should change roots_pem() to forcibly unwrap the results of its two calls instead, at which point it wouldn't need to return an optional. Would you mind adding that?
There was a problem hiding this comment.
Would you mind adding that?
Sure, thank you.
certificates: String? = nil, or (if that compiles, please try it and let me know if it doesn't)
It doesn't.
We should change roots_pem() access modifier to public to use it as a default value.
To be more specific, compiler fails with following error:
Global function 'roots_pem()' is internal and cannot be referenced from a default argument value.
If this way is inappropriate we can also do something like that to hide roots_pem:
enum RootCertificates {
case custom(String)
case `default`
}
But making roots_pem() public looks better to me.
There was a problem hiding this comment.
I think that we should definitely make roots_pem() public.
There was a problem hiding this comment.
Ok, so Roots.swift is a generated file which should not be edited according to comment in it.
I suggest to add a static method to gRPC class like that:
public final class gRPC {
/// Default root certificates provided by gRPC
public static let defaultRootCertificates = roots_pem()!
...
And after that, all should be ok finally.
There was a problem hiding this comment.
That file can be edited if you edit https://github.com/grpc/grpc-swift/blob/master/Sources/RootsEncoder/main.swift at the same time ;-)
There was a problem hiding this comment.
Got it! New commit ready for review.
Sources/SwiftGRPC/Core/Channel.swift
Outdated
| gRPC.initialize() | ||
| host = address | ||
| let argumentWrappers = arguments.map { $0.toCArg() } | ||
| var argumentValues = argumentWrappers.map { $0.wrapped } | ||
|
|
||
| underlyingChannel = cgrpc_channel_create_secure(address, certificates, clientCertificates, clientKey, &argumentValues, Int32(arguments.count)) | ||
| let certificatesToUs = certificates ?? roots_pem() |
There was a problem hiding this comment.
Would you mind renaming certificatesToUs to rootCertificates (or dropping it entirely if certificates: String = roots_pem() works)?
|
Requested changes made are made. |
|
As requested in #363 (comment), would you mind changing |
…nel API accordingly.
Got it. I've changed RootsEncoder's "main.swift", and generated a new one "Roots.swift" with RootsEncoder tool. Channel's initializer was changed accordingly. |
Allows a user to set up a channel with a client certificate and key but still use default root certificates.