Add support for validating server hostnames and IP addresses #171
Conversation
Lukasa
added
the
🔼 needs-minor-version-bump
Lukasa
force-pushed
the
cb-add-identity-policy
branch
from
dc5218e
to
9952a5d
Compare
| /// This policy implements the logic for service validation as specified by | ||
| /// RFC 6125 (https://tools.ietf.org/search/rfc6125), which loosely speaking | ||
| /// defines the common algorithm used for validating that an X.509 certificate | ||
| /// is valid for a given service |
There was a problem hiding this comment.
NIT
| /// is valid for a given service | |
| /// is valid for a given service. |
| /// - serverHostname: The hostname used to connect to the server. | ||
| /// - serverIP: The IP address of the server, if known. |
There was a problem hiding this comment.
Presumably you have to pass one or the other or both right? Can we enforce this in the type contract somehow?
There was a problem hiding this comment.
We can't, but I don't think it's necessary. If you pass neither, we'll fail to validate any cert.
| /// | ||
| /// Does nothing if the value is nil. | ||
| @inlinable | ||
| mutating func convert() -> ServerIdentityPolicy.IPAddress? { |
There was a problem hiding this comment.
NIT: Shouldn't this rather be an init on ServerIdentityPolicy.IPAdress that takes a ServerIdentityPolicy.LazyIPAddress and on the optional we can just call flatMap?
There was a problem hiding this comment.
No, because we modify this value as well.
Lukasa
force-pushed
the
cb-add-identity-policy
branch
12 times, most recently
from
b9dc9d0
to
bdc4aa0
Compare
|
|
||
| @inlinable | ||
| public mutating func chainMeetsPolicyRequirements(chain: UnverifiedCertificateChain) -> PolicyEvaluationResult { | ||
| let targetIP = self.serverIP.convert() |
There was a problem hiding this comment.
So this is caching the result. Why don't we do this just in the init? Is this laziness actually useful?
There was a problem hiding this comment.
The advantage of the laziness here is that if we never have to analyse a cert, we never do the construction.
| if let serverHostname = self.serverHostname, self.convertedServerHostname == nil { | ||
| self.convertedServerHostname = PreparedServerHostname(lowercaseASCIIBytes: serverHostname) | ||
| } |
There was a problem hiding this comment.
We are doing another way of caching here as well but forgot to set serverHostname to nil if the failable PreparedServerHostname.init returns nil. This means we are repeatedly trying to prepare the server hostname if it isn't a valid hostname. I suggest we move this to an init or model the internal state as an enum so we are sure we only do the initial conversion once.
There was a problem hiding this comment.
We can do the enum state, that works for me.
Lukasa
force-pushed
the
cb-add-identity-policy
branch
3 times, most recently
from
a668913
to
4a2fa6c
Compare
Motivation To make Swift Certificates useful for the Web PKI it's vital that we can validate that a particular certificate is authoritative for a specific domain. This requires a new verifier policy. Modifications Added ServerIdentityPolicy Added tests Result We can validate the identity of remote servers.
Lukasa
force-pushed
the
cb-add-identity-policy
branch
from
4a2fa6c
to
a87e4fb
Compare
|
Ah dang, merged too early. |
| } | ||
|
|
||
| let subsequentIndex = self.index(after: index) | ||
| return (self[..<index], self[subsequentIndex...]) |
There was a problem hiding this comment.
This will crash if index is self.endIndex
let array = [Int]()
_ = array.splitAroundIndex(array.endIndex) // Fatal error: Range requires lowerBound <= upperBound
Motivation
To make Swift Certificates useful for the Web PKI it's vital that we can validate that a particular certificate is authoritative for a specific domain. This requires a new verifier policy.
Modifications
Added ServerIdentityPolicy
Added tests
Result
We can validate the identity of remote servers.