Support download using Kerberos authentication

[stdweird]

Fixes #54
Requires quattor/CAF#145
Based on #88

[stdweird]

@ned21 @gombasg Can you have a look at the TODO? i was going to reuse the trust config, but that's not feasible, and i stumbled on some probably unintended behaviour

[stdweird]

Required steps to use this:

• kerberised webserver that forces TLS (kerberos only does authentication, you need to force TLS)
  • example httpd conf (similar to sindes one)
  • rewrite the request to the fqdn from the principal
  • allow aclmap to bypass the check, e.g. for the AII server

rewriteengine on
rewritemap ACLmap txt:/var/www/acl/ACLmap.txt
<directory "/var/www/quattor/profiles">
  AuthType GSSAPI
  AuthName "Quattor Kerberos Login"
  GssapiSSLonly On
  GssapiCredStore keytab:/etc/httpd/conf/ipa.keytab
  Require valid-user
  rewritecond ${ACLmap:%{REMOTE_USER}|NO} NO
  rewritecond %{REMOTE_USER} ^host/(.+)@YOUR.REALM$
  rewriterule ^(.*/)?.*\.(xml|json)(\.gz)?$ $1%1.$2$3 [L]
</directory>

• add CA to ccm.conf
• add principal to use to ccm.conf, typically host/f.q.d.n@REALM (ipa default)
  • add keytab if the principal cannot be retrieved from default one (ok for freeipa default)

TODO: verify TLS is enabled, have this reviewed by someone who has experience with apache configs

[ned21]

if ($self->{TRUST} && !grep($author =~ $_, @{$self->{TRUST}})) {
    die("Refusing profile generated by $author");
}

I can see how that looks like good Perl but leads to bad security policy. I suppose you could always constrain that $self->{TRUST} is always set in config but I am not sure that is the case. The lack of anchoring of the regex is also bad. I mostly agree with your formulation but not sure why you still need to check that @trusts is true given the previous line? I think the following should be sufficient?

my @trusts = @{$self->{TRUST} || []};
if (! grep($author eq $_, @trusts)) {
   die(...);
}

[stdweird]

@ned21 indeed, the @trusts is probably some leftover from some cleanup i tried to perform. it can all be replaced by

if (grep {$author =~ $_} @{$self->{TRUST} || [] }}) {
    $cnt = $payload;
} else {
    die("Refusing profile generated by $author");
}

do you want to tighten the regex also? i'm not sure how creative one can be in principals, but if you configure your trust as myserver.mydomain it will also match myserver.mydomain.notmydomain. or you want this enforced in the schema?

[ned21]

I think we should tighten the regex to match the exact principal only.

[stdweird]

@ned21 in this PR? i can make it

grep {$author =~ $_} map {"^$_\$"} @{$self->{TRUST} || [] }}

works fine like

$ perl -e 'my $auth = "ab"; print grep {$auth =~ $_} map {"^$_\$"} qw(a ab abc); print "\n";'
^ab$

[ned21]

Why use a regex instead of the eq operator?

$ perl -e 'my $auth = "ab"; print grep {$auth eq $_} qw(a ab abc); print "\n";'
ab

[stdweird]

@ned21 ofcourse. what do you currently use as trust? full principal incl the realm? i don't know what the $author can be, but it's from GSSAPI::Name->display so i'll assume it is. i'll modify the schema accordingly.

[ned21]

We use the full principal name including realm in our trust config.

[stdweird]

@ned21 ok, i'll use the eq in the grep and modifiy the docs/help text and the schema in ncm-ccm

[stdweird]

@ned21 looks like TRUST by default is empty arrayref. do you still want the die() when no match exists, or is $self->error and return; enough?

[ned21]

error() and return sounds less destructive than die()-ing? Very little else in CAF calls die() so I'm reluctant to start making things die() everywhere unless we are consistent about it.

[stdweird]

@ned21 to be clear: the die() code is the current code, and there are quite a few places in CAF that still call die, but afaik, it's only in CAF, rest of quattor code uses error/return

[ned21]

By "current" code do you mean what's used in ccm-fetch? In which case I would be inclined to keep it: error()/return makes sense in components where you want it to run to the end and report all errors. die() makes sense in ccm-fetch where you need to abort.

[stdweird]

@ned21 ok, i'll keep it. we can do the reevaluation of die another time.

[stdweird]

@ned21 modified, with tests

[ned21]

Thanks. One last request: can you please add a unit test where trust includes [email protected] (i.e. all lower case and no /service). That's what we actually specify in our configs and we were already bitten by some technically correct by over-zealous validation of the principal that required the realm to be upper case!

[stdweird]

@ned21 where did you run into this? ncm-ccm, the ccm.conf or CCM itself?

[ned21]

It was when validation of the principal name was added to ncm-ccm's schema I think.

[stdweird]

@ned21 so you want a test for all lowewrcase trust in ccm.conf in this repo and similar example in ncm-ccm?

[stdweird]

@ned21 test added

[ned21]

Thanks! What do we need to do to get the tests passing again?

[stdweird]

if you're lucky, merge the 2 PRs in the description 😄
if not, we need new buildtools first i think (i forgot what the current master needs, i'm testing with 10 PRs on top of master CAF, CCM and build-tools, there's no easy way back for my laptop atm 🙈 )

i'm adding the ncm-ccm unittest in quattor/configuration-modules-core#709 (as i managed to push the corresponding krb5 commit in that branch)

[stdweird]

@ned21 quattor/configuration-modules-core@35b3abd, component already covered it

[stdweird]

retest this please

[stdweird]

@ned21 tests pass now

[ned21]

Rebase please?

[jrha]

I've disabled the not up to date warnings from GitHub, it shouldn't complain unless there is actually a conflict now, it's unreasonable to expect all PRs to be rebased just because another has been merged.

[jrha]

Can be merged if @ned21 is happy!