[PATCH] Fix failure of mech plugins lacking gss_inquire_attrs_for_mech()

[David Woodhouse]

Since commit 030a4a03a ("Report inquire_attrs_For_mech mech failures")
the GSS-NTLMSSP plugin fails to work, because it doesn't provide a
gss_inquire_attrs_for_mech() method.

Sure, it can be fixed. But why do we need to break it? Isn't it
perfectly reasonable to assume that the answer is GSS_C_NO_OID_SET?

diff --git a/src/lib/gssapi/mechglue/g_mechattr.c b/src/lib/gssapi/mechglue/g_mechattr.c
index 08a6008..aa06319 100644
--- a/src/lib/gssapi/mechglue/g_mechattr.c
+++ b/src/lib/gssapi/mechglue/g_mechattr.c
@@ -181,14 +181,16 @@ gss_inquire_attrs_for_mech(
     mech = gssint_get_mechanism(selected_mech);
     if (mech == NULL)
         return GSS_S_BAD_MECH;
-    else if (mech->gss_inquire_attrs_for_mech == NULL)
-        return GSS_S_UNAVAILABLE;
-    public_mech = gssint_get_public_oid(selected_mech);
-    status = mech->gss_inquire_attrs_for_mech(minor, public_mech, mech_attrs,
-                                              known_mech_attrs);
-    if (GSS_ERROR(status)) {
-        map_error(minor, mech);
-        return status;
+
+    if (mech->gss_inquire_attrs_for_mech != NULL) {
+        public_mech = gssint_get_public_oid(selected_mech);
+        status = mech->gss_inquire_attrs_for_mech(minor, public_mech,
+                                                  mech_attrs,
+                                                  known_mech_attrs);
+        if (GSS_ERROR(status)) {
+            map_error(minor, mech);
+            return status;
+        }
     }
 
     if (known_mech_attrs != NULL && *known_mech_attrs == GSS_C_NO_OID_SET) {

[Greg Hudson]

Since commit 030a4a03a ("Report inquire_attrs_For_mech mech failures")
the GSS-NTLMSSP plugin fails to work, because it doesn't provide a
gss_inquire_attrs_for_mech() method.

I may have erred in accepting the new behavior, as RFC 5587 section
3.4.3 does not specify GSS_S_UNAVAILABLE as a return code.

However, I didn't like the old behavior either; it seems like a lie to
say "this mech has no attributes but knows about all of the attributes
from RFC 5587." The right answer might be to return GSS_S_COMPLETE but
supply an empty known_mech_attrs.

Either way, it's definitely a bug that gss_indicate_mechs_by_attr()
fails out in the presence of mechs which don't implement RFC 5587. It
ought to succeed, and it ought to include mechs which don't implement
RFC 5587 when called with empty desired and critical sets.
_______________________________________________
krbdev mailing list ***@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev

[David Woodhouse]

Post by Greg Hudson
However, I didn't like the old behavior either; it seems like a lie to
say "this mech has no attributes but knows about all of the attributes
from RFC 5587." 

I don't quite understand why we're doing that anyway.

If a mechanism has an inquire_attrs_for_mech() method which returns
GSS_C_NO_OID_SET for the known attrs, why would we override that and
assume that it *does* know everything in RFC5587 despite explicitly
telling us it doesn't?

The simple fix on the gssntlmssp side did precisely that:
https://bugzilla.redhat.com/show_bug.cgi?id=1317609#c3

With the krb5 code as it stands, that is equivalent to gssntlmssp
saying "I know all the RFC5587 attrs and I support none of  them".
Which is very different to "I know nothing but please don't reject me"
which is what it was *trying* to say.

[Greg Hudson]

Since commit 030a4a03a ("Report inquire_attrs_For_mech mech failures")
the GSS-NTLMSSP plugin fails to work, because it doesn't provide a
gss_inquire_attrs_for_mech() method.

If you can, please verify that
https://github.com/krb5/krb5/pull/426 fixes this scenario.
_______________________________________________
krbdev mailing list ***@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev

[David Woodhouse]

Post by Greg Hudson
If you can, please verify that
https://github.com/krb5/krb5/pull/426 fixes this scenario.

Yes, that fixes it. But I'm still unhappy. The "alternative" fix was to
implement gss_inquire_attrs_for_mech() in gssntlmssp, and just to
return GSS_C_NO_OID_SET for both supported and known mechanism sets. As
seen in http://david.woodhou.se/0001-Add-gss_inquire_attrs_for_mech.patch

With that patch to gssntmssp, it appears that the code in the generic
gss_inquire_attrs_for_mech() function is still going to override the
GSS_C_NO_OID_SET that I explicitly returned as the known_mech_attrs,
and wrongly assume that all attrs listed in RFC5587 are known.

Why?

[Greg Hudson]

Post by David Woodhouse
Yes, that fixes it. But I'm still unhappy. The "alternative" fix was to
implement gss_inquire_attrs_for_mech() in gssntlmssp, and just to
return GSS_C_NO_OID_SET for both supported and known mechanism sets. As
seen in http://david.woodhou.se/0001-Add-gss_inquire_attrs_for_mech.patch
With that patch to gssntmssp, it appears that the code in the generic
gss_inquire_attrs_for_mech() function is still going to override the
GSS_C_NO_OID_SET that I explicitly returned as the known_mech_attrs,
and wrongly assume that all attrs listed in RFC5587 are known.
Why?

I said most of this in the PR already, but: the mechglue implementation
of gss_inquire_attrs_for_mech() (which I did not write) includes a
convenience for mechanisms which understand exactly the RFC 5587
attributes and no more. The convenience is that if the mechanism leaves
(or explicitly sets) a null oid set in known_mech_attributes, the RFC
5587 attribute set will be placed there.

I do not particularly like this convenience, because it makes the SPI
different from the API without a good reason, and because it stops being
helpful as soon as a mechanism supports any attributes beyond the RFC
5587 set. But it would not necessarily be safe to remove the
convenience. The krb5 and SASL implementations of the function take
advantage of it, and it is possible that some loadable mechanisms do as
well.

All that aside, I don't believe that a mechanism should implement
gss_inquire_attrs_for_mech() without knowledge of the RFC 5587 attribute
set. If you really want to do so, you can create an empty-but-not-NULL
gss_OID_set for the known mechanism attributes, bypassing the mechglue
convenience feature.

I have pushed PR #426 to master.
_______________________________________________
krbdev mailing list ***@mit.edu
https://mailman.mit.edu/mailman/listinfo/krbdev