Re: [kitten] AD Review: draft-ietf-kitten-rfc5653bis-04

[Benjamin Kaduk <[e-mail]>]

On Fri, Jul 14, 2017 at 08:57:32AM -0700, Eric Rescorla wrote:
> On Wed, Jul 12, 2017 at 8:41 PM, Weijun Wang <[e-mail]> wrote:
> 
> > Hi Ekr
> >
> > Please read my answers below to your original questions.
> >
> > > On Jun 18, 2017, at 2:23 AM, Eric Rescorla <[e-mail]> wrote:
> > >
> > > This document seems generally sound. There are some things about this
> > > API that confused/surprised me and seem perhaps unwise, but given that
> > > this is a bis, I will mostly confine my review to mostly calling them
> > > out and asking you to make sure I understand and that the document is
> > > clear.
> > >
> > > OVERALL
> > > 1. What is a fatal error?
> > > The document describes exceptions as indicating "fatal errors".
> > > What does this mean for the state of the context. For instance,
> > > if I receive an exception from initSecContext(), does that mean
> > > that it is no longer possible to initiate it? Your example code
> > > seems to treat them as fatal for the context. What happens
> > > if I try to use a context after such an event?
> >
> > I’ll add a paragraph in "5.12.  Error Reporting” explaining whether the
> > context is useable after an exception is thrown, for context establishment
> > and per-message calls, respectively. Something like
> >
> > +If an exception is thrown during context establishment, the context
> > +negotiation has failed and the GSSContext object must be abandoned.
> > +If it is thrown in a per-message call, the context can remain useful.
> >
> > >
> > >
> > > 2. How do I enforce properties for received messages?
> > > I see that I can request services for context initialization
> > > (requestConf), and that I can check whether a given message
> > > was encrypted (getPrivacy) but it's not clear to me if this
> > > causes the API to enforce these rules for tokens that I
> > > receive. Is that possible or do I just need to check?
> >
> > You would have to check. Even if an established security context already
> > has its getConfState() being true, one can still wrap a message with
> > privacy state set to false and the peer will unwrap it with success. If the
> > peer insists only encrypted messages are allowed, she should always check.
> >
> > This is already documented in 6.4.10.
> >
> > >
> > > 3. Are the request* flags() hard limits? E.g., if I do
> > > requestMutualAuth() do I get it or fail?
> >
> > The method does not fail itself (i.e. does not throws an exception) and
> > you need to check the result with those getXyzState() methods after the
> > context is established.
> >
> > I can add a paragraph in S 5.9, something like:
> >
> > +If any retrieved attribute does not match the desired value
> > +but it is necessary for the application protocol, the application should
> > +destroy the security context and not use it for application traffic.
> > +Otherwise, at this point, the context can be used by the application to
> > +apply cryptographic services to its data.
> >
> 
> Sorry, I mean does the handshake fail? Or do you just hace to check.

You have to check.  The GSS-API has always been a request-and-check
type of API.

> 
> > 4. It's a little unusual to have a structure where you keep
> > > calling initSecContext or acceptContext() repeatedly. In
> > > most APIs you would do like "setRole(Server)" or
> > > "setRoleClient(), and then "Handshake().
> >
> > Sorry but this is how GSS-API works now.
> >
> > >
> > > DETAIL
> > > S 6.1.16.
> > > Can addProviderAtFront() be used to add new providers which
> > > the API would not normally use at all?
> >
> > No, 6.1.16 already had
> >
> >    Only when the indicated provider does not support
> >    the needed mechanism should the GSSManager move on to a different
> >    provider.
> >
> > I think this implies that a new provider added might be used at all.
> >
> 
> That doesn't seem very clear to me. My point is you might have defaults and
> then add new omnes.

I think you could use this to slot in your custom provider that was not
part of the system Java implementation, yes.
But I don't interact with the Java GSS stuff very much.


> > > EDITORIAL
> > > S 3.3.
> > > Please do not use the phrase "cryptographic checksum", I recognize
> > > that the terminology in this document is idiosyncratic because of
> > > age, but this isn't really a modern term. Typically
> > > we would now use "authentication tag" or "integrity tag”
> >
> > GSS-API is still using the Checksum word everywhere (RFC 3961’s title is
> > “Encryption and Checksum Specifications for Kerberos 5"). I think the
> > cryptographic word is added like we used in cryptographic random number
> > generator, which means it’s stronger than CRC etc. I would like to continue
> > using this word.
> >
> 
> At least add a parenthetical, or som other note, because this is not a term
> others can understand.

I agree; it's probably worth putting in a "(authentication tag)"
parenthetical.  (The parallel to a Kerberos Checksum remains only a
parallel, as Kerberos is only one of many GSS mechanisms.)


> > > S 5.3.
> > > gss_release_cred() is just eager, right? In any case the data will
> > > be cleaned up on GC? In any case you should make this clear.
> >
> > gss_release_cred() is eager, and there is no other guarantee the data can
> > be automatically cleaned up. Even if GC cleaned up the GSSCredential
> > object, there might be unreleased handles underneath.
> >
> > S 6.3 already has
> >
> >    When the credential is no longer needed, the application should call
> > the dispose (equivalent to gss_release_cred) method to release any
> > resources held by the credential object and to destroy any
> > cryptographically sensitive information.
> >
> > Do you think it’s necessary to append something like “An implementation
> > should not rely on garbage control or a finalize() method to dispose a
> > credential”?
> >
> 
> Yes.
> 
> 
> 
> >
> > > S 6.1.15.
> > > I wouldn't say you are "creating a previously exported context". You
> > > are either importing it or creating a new context from a previously
> > > exported one.
> >
> > Accepted.
> >
> > >
> > > S 6.2.1.
> > >
> > >    // export and re-import the name
> > >    byte[] exportName = mechName.export();
> > >
> > >    // create a new name object from the exported buffer
> > >    GSSName newName = mgr.createName(exportName,
> > >                      GSSName.NT_EXPORT_NAME);
> > >
> > > This comment structure is confusing, because the first is just
> > > the export. I would change that.
> >
> > Accepted.
> >
> > >
> > >
> > > S 6.2.6.
> > > It's a bit unclear to me under what circumstances you can compare GSS
> > > names. I see you can do .equals() and export/memcmp, but can you
> > > compare strings? Perhaps after you canonicalize them?
> >
> > As Ben and I explained this is quite complicated. Can we not touch it in
> > this bis?
> 
> 
> The fact that it's complicated seems like more reason to explain it.

In some sense, you can always compare GSS names (in that the compare-name
function will not throw an exception), but the results may not always
match the expected behavior.  (It's too bad that RFC 6680 didn't take the
time to summarize the state of affairs when adding naming extensions.)
I don't think we should try to provide a comprehensive review of the
state of affairs in this document, nor any normative statements (that would
only apply to implementations of the Java bindings whereas the issues
apply to all GSS-API implementations).  But perhaps something like the
following would be reasonable:

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

A full treatment of the behavior of GSS name comparison is outside the
scope of this work.  However, applications should note that to avoid
surpising behavior, it is best to ensure that the names being compared
are either both mechanism names for the same mechanism, or both internal
names that are not mechanism names.  This holds whether the .equals()
method is used directly, or the .export() method is used to generate byte
strings that are then compared byte-by-byte.

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

I considered adding a note that using .toString() and strcmp is
not recommended, but I do not think there is a non-normative statement
to be made that still provides value, and the lack of mention in the
text supplys a small disincentive to do so.

-Ben