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

[Eric Rescorla <[e-mail]>]

On Mon, Jul 17, 2017 at 5:22 PM, Benjamin Kaduk <[e-mail]> wrote:

> 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.
>

OK. I think that should be explicitly stated.


>
> >
> > > 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.
>

OK. This also needs to be explicit.

> > > 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.
>
> %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
>

Yes, this seems fine.

-Ekr