Re: [PATCH v20] GSSAPI encryption support

Greetings,

* Robbie Harwood (rharwood(at)redhat(dot)com) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > * Robbie Harwood (rharwood(at)redhat(dot)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.

Ok, that's easy enough to fix.

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

Agreed!

> Also, this is an MIT-specific extension: Heimdal doesn't support it.

Is that simply because they've not gotten around to it..? From what I
saw, this approach is in an accepted RFC, so if they want to implement
it, they could do so..?

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

We certainly don't want a hard MIT dependency, but if it's following an
RFC and we can gracefully fall-back if it isn't available then I think
it's acceptable. If there's a better approach which would work with
both MIT and Heimdal and ideally is defined through an RFC, that'd be
better, but I'm guessing there isn't...

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

Right, I'm aware that Kerberos could be using different encryption
algorithms, similar to SSL and that GSSAPI itself would allow for
different algorithms through different mechanisms- but the question is,
how do we know what the encryption algorithm ultimately used, regardless
of the rest, is?

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

I'm... not impressed with that approach, since "Kerberos" as an
encryption algorithm doesn't mean squat- that could be DES or RC4 for
all we know.

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

I agree that there's no sense in supporting 1DES-only.

I also, frankly, don't agree with the characterization that wanting to
know what the agreed-upon encryption algorithm is constitutes a gross
layering violation, but that's really an independent discussion that we
can have in a pub somewhere. ;)

To get where I'd like us to be, however, it sounds like we need to:

a) determine if we've got encryption (that's easy, assuming I've done it
correctly so far)

b) Figure out if the encryption is being provided by Kerberos (using
gss_inquire_context() would do that, right?)

c) If we're using Kerberos, use the lucid contexts to ask what the
actual encryption algorithm used is

That's a bit obnoxious but it at least seems doable. I don't suppose
you know of any example code out there which provides this? Any idea if
there's work underway on an RFC to make this, well, suck less?

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

This would be why I'd like to also provide the algorithm... :)

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

Good idea to also include mechanism explicitly.

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

Hmmm, that does look interesting, but I agree that it could be a
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.

I had been presuming that it'd be a buildsystem check, though it would
have been nice if it could have been something run-time.

> (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 have any issue with that provided we can implement the check in
a reasonable way (hint: I'm open to your suggestions on this :).

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

Fantastic. Do you think you'll have time to work through some of the
above with me over the next couple of weeks? I was just starting to
look into adding things into the beentry to hold information on the
server side.

Thanks!

Stephen