krb5 commit: Clear forwardable flag instead of denying request
Greg Hudson <[email protected]>
| Newsgroups | gmane.comp.encryption.kerberos.cvs |
|---|---|
| Message-ID | <[email protected]> |
https://github.com/krb5/krb5/commit/08e948cce2c79a3604066fcf7a64fc527456f83d commit 08e948cce2c79a3604066fcf7a64fc527456f83d Author: Greg Hudson <[email protected]> Date: Thu Nov 15 13:40:43 2018 -0500 Clear forwardable flag instead of denying request If the client requests a forwardable or proxiable ticket and the option cannot be honored by policy, issue a non-forwardable or non-proxiable ticket rather than denying the request. Add a test script for testing KDC request options and populate it with tests for the forwardable and proxiable flags. ticket: 7871 src/kdc/do_as_req.c | 19 ++------ src/kdc/do_tgs_req.c | 58 +++++--------------------- src/kdc/kdc_util.c | 82 +++++++++++++++++++++---------------- src/kdc/kdc_util.h | 9 ++-- src/kdc/tgs_policy.c | 8 +--- src/tests/Makefile.in | 1 + src/tests/gcred.c | 28 +++++++++---- src/tests/t_kdcoptions.py | 100 +++++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 190 insertions(+), 115 deletions(-) diff --git a/src/kdc/do_as_req.c b/src/kdc/do_as_req.c index 588c137..8a96c12 100644 --- a/src/kdc/do_as_req.c +++ b/src/kdc/do_as_req.c @@ -192,13 +192,6 @@ finish_process_as_req(struct as_req_state *state, krb5_error_code errcode) au_state->stage = ENCR_REP; - if ((errcode = validate_forwardable(state->request, *state->client, - *state->server, state->kdc_time, - &state->status))) { - errcode += ERROR_TABLE_BASE_krb5; - goto egress; - } - errcode = check_indicators(kdc_context, state->server, state->auth_indicators); if (errcode) { @@ -708,12 +701,11 @@ process_as_req(krb5_kdc_req *request, krb5_data *req_pkt, } /* Copy options that request the corresponding ticket flags. */ - state->enc_tkt_reply.flags = OPTS2FLAGS(state->request->kdc_options); + state->enc_tkt_reply.flags = get_ticket_flags(state->request->kdc_options, + state->client, state->server, + NULL); state->enc_tkt_reply.times.authtime = state->authtime; - setflag(state->enc_tkt_reply.flags, TKT_FLG_INITIAL); - setflag(state->enc_tkt_reply.flags, TKT_FLG_ENC_PA_REP); - /* * It should be noted that local policy may affect the * processing of any of these flags. For example, some @@ -732,10 +724,9 @@ process_as_req(krb5_kdc_req *request, krb5_data *req_pkt, state->enc_tkt_reply.transited.tr_type = KRB5_DOMAIN_X500_COMPRESS; state->enc_tkt_reply.transited.tr_contents = empty_string; - if (isflagset(state->request->kdc_options, KDC_OPT_POSTDATED)) { - setflag(state->enc_tkt_reply.flags, TKT_FLG_INVALID); + if (isflagset(state->request->kdc_options, KDC_OPT_POSTDATED)) state->enc_tkt_reply.times.starttime = state->request->from; - } else + else state->enc_tkt_reply.times.starttime = state->kdc_time; kdc_get_ticket_endtime(kdc_active_realm, diff --git a/src/kdc/do_tgs_req.c b/src/kdc/do_tgs_req.c index 587342a..1da0993 100644 --- a/src/kdc/do_tgs_req.c +++ b/src/kdc/do_tgs_req.c @@ -378,15 +378,16 @@ process_tgs_req(krb5_kdc_req *request, krb5_data *pkt, else ticket_reply.server = request->server; /* XXX careful for realm... */ - enc_tkt_reply.flags = OPTS2FLAGS(request->kdc_options); - enc_tkt_reply.flags |= COPY_TKT_FLAGS(header_enc_tkt->flags); + enc_tkt_reply.flags = get_ticket_flags(request->kdc_options, client, + server, header_enc_tkt); enc_tkt_reply.times.starttime = 0; - if (isflagset(server->attributes, KRB5_KDB_OK_AS_DELEGATE)) - setflag(enc_tkt_reply.flags, TKT_FLG_OK_AS_DELEGATE); - - /* Indicate support for encrypted padata (RFC 6806). */ - setflag(enc_tkt_reply.flags, TKT_FLG_ENC_PA_REP); + /* OK_TO_AUTH_AS_DELEGATE must be set on the service requesting S4U2Self + * for forwardable tickets to be issued. */ + if (isflagset(c_flags, KRB5_KDB_FLAG_PROTOCOL_TRANSITION) && + !is_referral && + !isflagset(server->attributes, KRB5_KDB_OK_TO_AUTH_AS_DELEGATE)) + clear(enc_tkt_reply.flags, TKT_FLG_FORWARDABLE); /* don't use new addresses unless forwarded, see below */ @@ -401,37 +402,6 @@ process_tgs_req(krb5_kdc_req *request, krb5_data *pkt, * realms may refuse to issue renewable tickets */ - if (isflagset(request->kdc_options, KDC_OPT_FORWARDABLE)) { - - if (isflagset(c_flags, KRB5_KDB_FLAG_PROTOCOL_TRANSITION)) { - /* - * If S4U2Self principal is not forwardable, then mark ticket as - * unforwardable. This behaviour matches Windows, but it is - * different to the MIT AS-REQ path, which returns an error - * (KDC_ERR_POLICY) if forwardable tickets cannot be issued. - * - * Consider this block the S4U2Self equivalent to - * validate_forwardable(). - */ - if (client != NULL && - isflagset(client->attributes, KRB5_KDB_DISALLOW_FORWARDABLE)) - clear(enc_tkt_reply.flags, TKT_FLG_FORWARDABLE); - /* - * Forwardable flag is propagated along referral path. - */ - else if (!isflagset(header_enc_tkt->flags, TKT_FLG_FORWARDABLE)) - clear(enc_tkt_reply.flags, TKT_FLG_FORWARDABLE); - /* - * OK_TO_AUTH_AS_DELEGATE must be set on the service requesting - * S4U2Self in order for forwardable tickets to be returned. - */ - else if (!is_referral && - !isflagset(server->attributes, - KRB5_KDB_OK_TO_AUTH_AS_DELEGATE)) - clear(enc_tkt_reply.flags, TKT_FLG_FORWARDABLE); - } - } - if (isflagset(request->kdc_options, KDC_OPT_FORWARDED) || isflagset(request->kdc_options, KDC_OPT_PROXY)) { @@ -440,16 +410,10 @@ process_tgs_req(krb5_kdc_req *request, krb5_data *pkt, enc_tkt_reply.caddrs = request->addresses; reply_encpart.caddrs = request->addresses; } - /* We don't currently handle issuing anonymous tickets based on - * non-anonymous ones, so just ignore the option. */ - if (isflagset(request->kdc_options, KDC_OPT_REQUEST_ANONYMOUS) && - !isflagset(header_enc_tkt->flags, TKT_FLG_ANONYMOUS)) - clear(enc_tkt_reply.flags, TKT_FLG_ANONYMOUS); - - if (isflagset(request->kdc_options, KDC_OPT_POSTDATED)) { - setflag(enc_tkt_reply.flags, TKT_FLG_INVALID); + + if (isflagset(request->kdc_options, KDC_OPT_POSTDATED)) enc_tkt_reply.times.starttime = request->from; - } else + else enc_tkt_reply.times.starttime = kdc_time; if (isflagset(request->kdc_options, KDC_OPT_VALIDATE)) { diff --git a/src/kdc/kdc_util.c b/src/kdc/kdc_util.c index dfeaf7e..6d53173 100644 --- a/src/kdc/kdc_util.c +++ b/src/kdc/kdc_util.c @@ -697,29 +697,6 @@ validate_as_request(kdc_realm_t *kdc_active_realm, return(KDC_ERR_CANNOT_POSTDATE); } - /* - * A Windows KDC will return KDC_ERR_PREAUTH_REQUIRED instead of - * KDC_ERR_POLICY in the following case: - * - * - KDC_OPT_FORWARDABLE is set in KDCOptions but local - * policy has KRB5_KDB_DISALLOW_FORWARDABLE set for the - * client, and; - * - KRB5_KDB_REQUIRES_PRE_AUTH is set for the client but - * preauthentication data is absent in the request. - * - * Hence, this check most be done after the check for preauth - * data, and is now performed by validate_forwardable() (the - * contents of which were previously below). - */ - - /* Client and server must allow proxiable tickets */ - if (isflagset(request->kdc_options, KDC_OPT_PROXIABLE) && - (isflagset(client.attributes, KRB5_KDB_DISALLOW_PROXIABLE) || - isflagset(server.attributes, KRB5_KDB_DISALLOW_PROXIABLE))) { - *status = "PROXIABLE NOT ALLOWED"; - return(KDC_ERR_POLICY); - } - /* Check to see if client is locked out */ if (isflagset(client.attributes, KRB5_KDB_DISALLOW_ALL_TIX)) { *status = "CLIENT LOCKED OUT"; @@ -752,19 +729,54 @@ validate_as_request(kdc_realm_t *kdc_active_realm, return 0; } -int -validate_forwardable(krb5_kdc_req *request, krb5_db_entry client, - krb5_db_entry server, krb5_timestamp kdc_time, - const char **status) +/* + * Compute ticket flags based on the request, the client and server DB entry + * (which may prohibit forwardable or proxiable tickets), and the header + * ticket. client may be NULL for a TGS request (although it may be set, such + * as for an S4U2Self request). header_enc may be NULL for an AS request. + */ +krb5_flags +get_ticket_flags(krb5_flags reqflags, krb5_db_entry *client, + krb5_db_entry *server, krb5_enc_tkt_part *header_enc) { - *status = NULL; - if (isflagset(request->kdc_options, KDC_OPT_FORWARDABLE) && - (isflagset(client.attributes, KRB5_KDB_DISALLOW_FORWARDABLE) || - isflagset(server.attributes, KRB5_KDB_DISALLOW_FORWARDABLE))) { - *status = "FORWARDABLE NOT ALLOWED"; - return(KDC_ERR_POLICY); - } else - return 0; + krb5_flags flags; + + /* Indicate support for encrypted padata (RFC 6806), and set flags based on + * request options and the header ticket. */ + flags = OPTS2FLAGS(reqflags) | TKT_FLG_ENC_PA_REP; + if (reqflags & KDC_OPT_POSTDATED) + flags |= TKT_FLG_INVALID; + if (header_enc != NULL) + flags |= COPY_TKT_FLAGS(header_enc->flags); + if (header_enc == NULL) + flags |= TKT_FLG_INITIAL; + + /* For TGS requests, indicate if the service is marked ok-as-delegate. */ + if (header_enc != NULL && (server->attributes & KRB5_KDB_OK_AS_DELEGATE)) + flags |= TKT_FLG_OK_AS_DELEGATE; + + /* Unset PROXIABLE if it is disallowed. */ + if (client != NULL && (client->attributes & KRB5_KDB_DISALLOW_PROXIABLE)) + flags &= ~TKT_FLG_PROXIABLE; + if (server->attributes & KRB5_KDB_DISALLOW_PROXIABLE) + flags &= ~TKT_FLG_PROXIABLE; + if (header_enc != NULL && !(header_enc->flags & TKT_FLG_PROXIABLE)) + flags &= ~TKT_FLG_PROXIABLE; + + /* Unset FORWARDABLE if it is disallowed. */ + if (client != NULL && (client->attributes & KRB5_KDB_DISALLOW_FORWARDABLE)) + flags &= ~TKT_FLG_FORWARDABLE; + if (server->attributes & KRB5_KDB_DISALLOW_FORWARDABLE) + flags &= ~TKT_FLG_FORWARDABLE; + if (header_enc != NULL && !(header_enc->flags & TKT_FLG_FORWARDABLE)) + flags &= ~TKT_FLG_FORWARDABLE; + + /* We don't currently handle issuing anonymous tickets based on + * non-anonymous ones. */ + if (header_enc != NULL && !(header_enc->flags & TKT_FLG_ANONYMOUS)) + flags &= ~TKT_FLG_ANONYMOUS; + + return flags; } /* Return KRB5KDC_ERR_POLICY if indicators does not contain the required auth diff --git a/src/kdc/kdc_util.h b/src/kdc/kdc_util.h index 6ec645f..b89a3de 100644 --- a/src/kdc/kdc_util.h +++ b/src/kdc/kdc_util.h @@ -86,15 +86,14 @@ validate_as_request (kdc_realm_t *, krb5_kdc_req *, krb5_db_entry, const char **, krb5_pa_data ***); int -validate_forwardable(krb5_kdc_req *, krb5_db_entry, - krb5_db_entry, krb5_timestamp, - const char **); - -int validate_tgs_request (kdc_realm_t *, krb5_kdc_req *, krb5_db_entry, krb5_ticket *, krb5_timestamp, const char **, krb5_pa_data ***); +krb5_flags +get_ticket_flags(krb5_flags reqflags, krb5_db_entry *client, + krb5_db_entry *server, krb5_enc_tkt_part *header_enc); + krb5_error_code check_indicators(krb5_context context, krb5_db_entry *server, krb5_data *const *indicators); diff --git a/src/kdc/tgs_policy.c b/src/kdc/tgs_policy.c index 907fcd3..554345b 100644 --- a/src/kdc/tgs_policy.c +++ b/src/kdc/tgs_policy.c @@ -63,9 +63,9 @@ static check_tgs_svc_pol_fn * const svc_pol_fns[] = { }; static const struct tgsflagrule tgsflagrules[] = { - { (KDC_OPT_FORWARDED | KDC_OPT_FORWARDABLE), TKT_FLG_FORWARDABLE, + { KDC_OPT_FORWARDED, TKT_FLG_FORWARDABLE, "TGT NOT FORWARDABLE", KDC_ERR_BADOPTION }, - { (KDC_OPT_PROXY | KDC_OPT_PROXIABLE), TKT_FLG_PROXIABLE, + { KDC_OPT_PROXY, TKT_FLG_PROXIABLE, "TGT NOT PROXIABLE", KDC_ERR_BADOPTION }, { (KDC_OPT_ALLOW_POSTDATE | KDC_OPT_POSTDATED), TKT_FLG_MAY_POSTDATE, "TGT NOT POSTDATABLE", KDC_ERR_BADOPTION }, @@ -98,12 +98,8 @@ check_tgs_opts(krb5_kdc_req *req, krb5_ticket *tkt, const char **status) } static const struct tgsflagrule svcdenyrules[] = { - { KDC_OPT_FORWARDABLE, KRB5_KDB_DISALLOW_FORWARDABLE, - "NON-FORWARDABLE TICKET", KDC_ERR_POLICY }, { KDC_OPT_RENEWABLE, KRB5_KDB_DISALLOW_RENEWABLE, "NON-RENEWABLE TICKET", KDC_ERR_POLICY }, - { KDC_OPT_PROXIABLE, KRB5_KDB_DISALLOW_PROXIABLE, - "NON-PROXIABLE TICKET", KDC_ERR_POLICY }, { KDC_OPT_ALLOW_POSTDATE, KRB5_KDB_DISALLOW_POSTDATED, "NON-POSTDATABLE TICKET", KDC_ERR_CANNOT_POSTDATE }, { KDC_OPT_ENC_TKT_IN_SKEY, KRB5_KDB_DISALLOW_DUP_SKEY, diff --git a/src/tests/Makefile.in b/src/tests/Makefile.in index e27617e..0eec2b6 100644 --- a/src/tests/Makefile.in +++ b/src/tests/Makefile.in @@ -177,6 +177,7 @@ check-pytests: unlockiter $(RUNPYTEST) $(srcdir)/t_y2038.py $(PYTESTFLAGS) $(RUNPYTEST) $(srcdir)/t_kdcpolicy.py $(PYTESTFLAGS) $(RUNPYTEST) $(srcdir)/t_u2u.py $(PYTESTFLAGS) + $(RUNPYTEST) $(srcdir)/t_kdcoptions.py $(PYTESTFLAGS) clean: $(RM) adata etinfo forward gcred hist hooks hrealm icinterleave icred diff --git a/src/tests/gcred.c b/src/tests/gcred.c index cb0ae6a..b14e4fc 100644 --- a/src/tests/gcred.c +++ b/src/tests/gcred.c @@ -66,20 +66,32 @@ main(int argc, char **argv) krb5_principal client, server; krb5_ccache ccache; krb5_creds in_creds, *creds; + krb5_flags options = 0; char *name; + int c; check(krb5_init_context(&ctx)); - /* Parse arguments. */ - assert(argc == 3); - check(krb5_parse_name(ctx, argv[2], &server)); - if (strcmp(argv[1], "unknown") == 0) + while ((c = getopt(argc, argv, "f")) != -1) { + switch (c) { + case 'f': + options |= KRB5_GC_FORWARDABLE; + break; + default: + abort(); + } + } + argc -= optind; + argv += optind; + assert(argc == 2); + check(krb5_parse_name(ctx, argv[1], &server)); + if (strcmp(argv[0], "unknown") == 0) server->type = KRB5_NT_UNKNOWN; - else if (strcmp(argv[1], "principal") == 0) + else if (strcmp(argv[0], "principal") == 0) server->type = KRB5_NT_PRINCIPAL; - else if (strcmp(argv[1], "srv-inst") == 0) + else if (strcmp(argv[0], "srv-inst") == 0) server->type = KRB5_NT_SRV_INST; - else if (strcmp(argv[1], "srv-hst") == 0) + else if (strcmp(argv[0], "srv-hst") == 0) server->type = KRB5_NT_SRV_HST; else abort(); @@ -89,7 +101,7 @@ main(int argc, char **argv) memset(&in_creds, 0, sizeof(in_creds)); in_creds.client = client; in_creds.server = server; - check(krb5_get_credentials(ctx, 0, ccache, &in_creds, &creds)); + check(krb5_get_credentials(ctx, options, ccache, &in_creds, &creds)); check(krb5_unparse_name(ctx, creds->server, &name)); printf("%s\n", name); diff --git a/src/tests/t_kdcoptions.py b/src/tests/t_kdcoptions.py new file mode 100644 index 0000000..7ec5750 --- /dev/null +++ b/src/tests/t_kdcoptions.py @@ -0,0 +1,100 @@ +from k5test import * +import re + +# KDC option test coverage notes: +# +# FORWARDABLE here +# FORWARDED no test +# PROXIABLE here +# PROXY no test +# ALLOW_POSTDATE no test +# POSTDATED no test +# RENEWABLE t_renew.py +# CNAME_IN_ADDL_TKT gssapi/t_s4u.py +# CANONICALIZE t_kdb.py and various other tests +# REQUEST_ANONYMOUS t_pkinit.py +# DISABLE_TRANSITED_CHECK no test +# RENEWABLE_OK t_renew.py +# ENC_TKT_IN_SKEY t_u2u.py +# RENEW t_renew.py +# VALIDATE no test + +# Run klist -f and return the flags on the ticket for svcprinc. +def get_flags(realm, svcprinc): + grab_flags = False + for line in realm.run([klist, '-f']).splitlines(): + if grab_flags: + return re.findall(r'Flags: ([a-zA-Z]*)', line)[0] + grab_flags = line.endswith(svcprinc) + + +# Get the flags on the ticket for svcprinc, and check for an expected +# element and an expected-absent element, either of which can be None. +def check_flags(realm, svcprinc, expected_flag, expected_noflag): + flags = get_flags(realm, svcprinc) + if expected_flag is not None and not expected_flag in flags: + fail('expected flag ' + expected_flag) + if expected_noflag is not None and expected_noflag in flags: + fail('did not expect flag ' + expected_noflag) + + +# Run kinit with the given flags, and check the flags on the resulting +# TGT. +def kinit_check_flags(realm, flags, expected_flag, expected_noflag): + realm.kinit(realm.user_princ, password('user'), flags) + check_flags(realm, realm.krbtgt_princ, expected_flag, expected_noflag) + + +# Run kinit with kflags. Then get credentials for the host principal +# with gflags, and check the flags on the resulting ticket. +def gcred_check_flags(realm, kflags, gflags, expected_flag, expected_noflag): + realm.kinit(realm.user_princ, password('user'), kflags) + realm.run(['./gcred'] + gflags + ['unknown', realm.host_princ]) + check_flags(realm, realm.host_princ, expected_flag, expected_noflag) + + +realm = K5Realm() + +mark('proxiable (AS)') +kinit_check_flags(realm, [], None, 'P') +kinit_check_flags(realm, ['-p'], 'P', None) +realm.run([kadminl, 'modprinc', '-allow_proxiable', realm.user_princ]) +kinit_check_flags(realm, ['-p'], None, 'P') +realm.run([kadminl, 'modprinc', '+allow_proxiable', realm.user_princ]) +realm.run([kadminl, 'modprinc', '-allow_proxiable', realm.krbtgt_princ]) +kinit_check_flags(realm, ['-p'], None, 'P') +realm.run([kadminl, 'modprinc', '+allow_proxiable', realm.krbtgt_princ]) + +mark('proxiable (TGS)') +gcred_check_flags(realm, [], [], None, 'P') +gcred_check_flags(realm, ['-p'], [], 'P', None) + +# Not tested: PROXIABLE option set with a non-proxiable TGT (because +# there is no krb5_get_credentials() flag to request this; would +# expect a non-proxiable ticket). + +# Not tested: proxiable TGT but PROXIABLE flag not set (because we +# internally set the PROXIABLE option when using a proxiable TGT; +# would expect a non-proxiable ticket). + +mark('forwardable (AS)') +kinit_check_flags(realm, [], None, 'F') +kinit_check_flags(realm, ['-f'], 'F', None) +realm.run([kadminl, 'modprinc', '-allow_forwardable', realm.user_princ]) +kinit_check_flags(realm, ['-f'], None, 'F') +realm.run([kadminl, 'modprinc', '+allow_forwardable', realm.user_princ]) +realm.run([kadminl, 'modprinc', '-allow_forwardable', realm.krbtgt_princ]) +kinit_check_flags(realm, ['-f'], None, 'F') +realm.run([kadminl, 'modprinc', '+allow_forwardable', realm.krbtgt_princ]) + +mark('forwardable (TGS)') +realm.kinit(realm.user_princ, password('user')) +gcred_check_flags(realm, [], [], None, 'F') +gcred_check_flags(realm, [], ['-f'], None, 'F') +gcred_check_flags(realm, ['-f'], [], 'F', None) + +# Not tested: forwardable TGT but FORWARDABLE flag not set (because we +# internally set the FORWARDABLE option when using a forwardable TGT; +# would expect a non-proxiable ticket). + +success('KDC option tests')