Re: [PATCH v20] GSSAPI encryption support

Stephen Frost <sfrost@snowman.net> writes:

> * Robbie Harwood (rharwood@redhat.com) wrote:
>
>> Attached please find version 20 of the GSSAPI encryption support.
>> This has been rebased onto master (thanks Stephen for calling out
>> ab69ea9).
>
> I've looked over this again and have been playing with it off-and-on
> for a while now.  One thing I was actually looking at implementing
> before we push to commit this was to have similar information to what
> SSL provides on connection, specifically what printSSLInfo() prints by
> way of cipher, bits, etc for the client-side and then something like
> pg_stat_ssl for the server side.
>
> I went ahead and added a printGSSENCInfo(), and then
> PQgetgssSASLSSF(), which calls down into
> gss_inquire_sec_context_by_oid() for GSS_C_SEC_CONTEXT_SASL_SSF to get
> the bits (which also required including gssapi_ext.h), and now have
> psql producing:

Neat!  Two things here:

First, because this is a SASL extension, it requires existing mechanism
integration with SASL.  For instance, NTLM (through gss-ntlmssp) doesn't
implement it.  PQgetgssSASLSSF() logs an error message here, which I
don't think is quite right - probably you should only log an error
message if you don't get back GSS_S_COMPLETE or GSS_S_UNAVAILABLE.

(NTLM is an example here; I cannot in good conscience recommend its use,
and neither does Microsoft.)

Also, this is an MIT-specific extension: Heimdal doesn't support it.
While I (a krb5 developer) am fine with a hard MIT dependency, the
project doesn't currently have this.  (And if we went that road, I'd
like to use gss_acquire_cred_from() instead of the setenv() in
be-secure-gssapi.c.)

> ---
> psql (12devel)
> GSS Encrypted connection (bits: 256)
> Type "help" for help.
> ---
>
> I was curious though- is there a good way to get at the encryption
> type used..?  I haven't had much luck in reading the RFPs and poking
> around, but perhaps I'm just not looking in the right place.

It's something of a layering issue.  GSSAPI might not be using Kerberos,
and even if it is, Kerberos has algorithm agility.

A call to gss_inquire_context() produces mech type, so you could print
something like "GSS Encrypted connection (Kerberos 256 bits)" or "GSS
encrypted connection (NTLM)".  I think this'd be pretty reasonable.

For Kerberos-specific introspection, MIT krb5 and Heimdal both support
an interface called lucid contexts that allows this kind of
introspection (and some other gross layering violations) for use with
the kernel.  Look at gssapi_krb5.h (it's called that in both).  I don't
think it's worth supporting rfc1964 at this point (it's 1DES-only).

> Also, what information do you think would be particularly useful on
> the server side?  I was thinking that the princ used to authenticate,
> the encryption type/cipher, and bits used, at least, would be
> meaningful.

I'm wary about indicating number of bits especially with the
asymmetric/symmetric divide and the importance of which algorithm
they're used with, but that may be a non-concern - especially if it
attains parity with the TLS code.

Principal used is definitely good (helps debugging the user mapping
rules, if nothing else).  Mechanism is also nice.  I can't think of
anything else right now that you haven't mentioned.

I think there's value in auth indcatorsp
http://web.mit.edu/kerberos/krb5-latest/doc/admin/auth_indicator.html
but supporting them would be a separate follow-on patch.

> I'm also guessing that we may need to add something to
> make these functions not fail on older systems which don't provide the
> SASL SSF OID..?  I haven't looked into that yet but we should.

MIT krb5 gained support in 2017, which corresponds to krb5-1.16; Heimdal
has no support for it.  There probably isn't a shortcut for a
buildsystem check, unfortunately: Heimdal has
gss_inquire_sec_context_by_oid() in another file (they don't have
gssapi_ext.h), and it's the OID is MIT-only and not a #define-d
constant.

(With my downstream maintainer hat on, I'd further ask that any such
check not just be a version check in order to allow backporting; in
particular, the el7 krb5-1.15 branch does include support for this OID.)

> I don't think these things are absolutely required to push the patch
> forward, just to be clear, but it's helping my confidence level, at
> least, and would make it closer to on-par with our current SSL
> implementation.  They also seem, well, reasonably straight-forward to
> add and quite useful.

Auditability is definitely reasonable.  I think there are a couple
additional wrinkles discussed above, but we can probably get them
sorted.

> I've attached my WIP patch for adding the bits, patched over-top of your
> v20 (would be neat if there was a way to tell cfbot to ignore it...).

I don't have additional concerns beyond the above.

Thanks,
--Robbie