On 7/13/2018 1:35 PM, Xuelei Fan wrote:
I think we need to check more aspects, for both the session resumption
producer and consumer:
1. the local is able to resume the session.
2. the peer is able to resume the session.
3. the change of the security parameters does not impact the resumption.
1 and 3 I agree with, and I believe are pretty well covered (after this
change), but I'm still not convinced that the implementation should do
any checking on behalf of the peer. This includes checking the peer
supported signature algorithms. Maybe this is a good idea, but I think
it is not in scope of this ticket, and it should be discussed
separately. If you want to pursue this, feel free to open another ticket
for it.
So, for the protocol version checking in server side:
1. the client requested to use the negotiated version.
2. the server still supports the negotiated version.
I think #2 is get checked in line 406-414 block.
Yes.
Did we check #1 in somewhere else?
A lot of the checks for the client side uses existing code, and is
shared between TLS 1.3 and earlier versions. The way it works is that
the shared code will decide which session to resume in ClientHello. At
this point, a lot of things are checked, including the protocol version
of the session. Only if this code decides that a session should be
resumed, then the code for TLS 1.3+ in PreSharedKeyExtension will
perform the additional checks that are specific to TLS 1.3 and then
produce the extension.
Specifically, the protocol version is checked in ClientHello.java near
line 458 (in existing code).
For the cipher suite checking in server side:
1. the client requested to use the negotiate cipher suite.
2. the server still supports the negotiated cipher suite.
I think #2 is get checked in line 445-453 block.
Yes.
Did we check #1 in somewhere else?
ClientHello.java near line 445 (in existing code).
For the signature algorithms checking in server side:
1. the client requested to use the algorithms used in full handshake
2. the server still supports the algorithms used in full handshake
The algorithms used in full handshake is one element or a subset of
the local and peer supported algorithms. We may not cache the used
algorithms in the session. But it is acceptable to me to use
defensive policy that we don't allow session resumption if the
supported algorithms get changed (as you did in the webrev to use
Collection.containsAll() line 435).
We do cache the certificate chain. If we wanted to, we could examine the
chain and make sure all of the signature algorithms in the chain are
still supported. I didn't think this was worth the effort, though.
As reminded me that we may also want to check for both the
signature_algorithms and signature_algorithms algorithms.
Did you mean signature_algorithms and signature_algorithms_cert? I think
that is handled correctly now, because the set of (local) supported
signature algorithms for certificates and handshake messages will always
be the same (right?). We only have one field for this in
HandshakeContext: localSupportedSignAlgs. The way it is used in the
code, this field is used for both handshake messages and certificates.
I think #2 is get checked in line 431-442 block.
Yes.
Did we check #1 in somewhere else?
Because this is specific to TLS 1.3+, I put it in the
CHPreSharedKeyProducer. That is in PreSharedKeyExtension.java, near line
610 (in the new code in the Webrev).
On 7/13/2018 9:10 AM, Adam Petcher wrote:
On 7/13/2018 11:34 AM, Xuelei Fan wrote:
PreSharedKeyExtension.java
--------------------------
The local supported signature algorithms are checked in the
canRejoin() method. Should the peer supported signature algorithms
be checked as well?
I don't think so. When the peer creates its PreSharedKeyExtension, it
should only offer sessions (i.e. PSK identities) that it is willing
to resume. This includes checking for its supported signature
algorithms, or any checks that are required by its policy.
I see your point. But the peer is not trustworthy so that we normally
validate the input to make sure the peer is doing the right thing.
Thanks,
Xuelei
If the server gets a PSK identity from the client, then server should
use that PSK to resume a session as long as it is acceptable
according to the server's policy. Trying to figure out the peer's
policy and enforce it is error prone and adds unnecessary complexity.
Though maybe I'm missing some other motivation to add this check.
