KDC status strings consistency
Zhanna Tsitkov
tsitkova at MIT.EDU
Wed Oct 9 13:15:27 EDT 2013
kdc_status branch is now updated.
Thanks,
Zhanna
On Sep 30, 2013, at 5:37 PM, Tom Yu wrote:
> I would appreciate it if others who have operational experience can
> look at whether any of these changes adversely affect log analysis or
> similar operations. In particular, I think it's possible that the
> status strings involving anonymous requests are somewhat more likely
> to be seen by users.
>
> Zhanna Tsitkov <tsitkova at MIT.EDU> writes:
>
>> I'm suggesting to change some KDC status strings to make them more
>> consistent.
>> Please, take a look the proposed changes at https://github.com/tsitkov/krb5/tree/kdc_status
>
> If we are going to try to make the KDC status strings more consistent,
> we should document the scheme somewhere more accessible than in a
> commit message. One such location could be in the comment block for
> log_as_req() in kdc_util.c.
Done. In addition I suggest to move all KDC logging routines into a
separate file kdc_log.c.
>
> We could also use more clarity about the guidelines for status
> strings. I agree with you that we should use capitalized phrases with
> underscores as separators, and that we should prefer to make them
> imperative phrases. I think we should also make the status strings
> reflect the name of the function that encountered the error.
I don't think that the KDC status strings should correlate with the
function names. We want to log the "state of the KDC" rather than the
name of the function, that would be useful for admins, auditors etc.
>
> More specific comments are below.
>
>
> commit c94ee844e80b69ba97dec6c2e72d965ff4c91234
> Author: Zhanna Tsitkov <tsitkova at mit.edu>
> Date: Fri Sep 27 13:46:37 2013 -0400
>
> Make KDC "status" statements more homogeneous
>
> Generally we want KDC status strings to be concise, informative
> and follow
> some common rule. Many of the current status messages are in
> "DO_THIS" format,
> i.e. uppercased string with underscore as a separator. For example,
> "DECRYPT_SERVER_KEY". This commit is to modify some KDC status
> messages to
> follow this format.
>
> Even though KDC status messages are not standardized, it is
> possible that some
> administrators use them in the Kerberos log file processing.
> Hence, the vast
> majority of them are left unchanged pending further investigation
> (mostly,
> feedback from the administrators).
>
> diff --git a/src/kdc/do_as_req.c b/src/kdc/do_as_req.c
> index 0368407..6c331c5 100644
> --- a/src/kdc/do_as_req.c
> +++ b/src/kdc/do_as_req.c
> @@ -215,7 +215,7 @@ finish_process_as_req(struct as_req_state
> *state, krb5_error_code errcode)
> state->reply_encpart.session = &state->session_key;
> if ((errcode = fetch_last_req_info(state->client,
> &state-
> >reply_encpart.last_req))) {
> - state->status = "FETCH_LAST_REQ";
> + state->status = "FETCH_LAST_REQUEST";
> goto egress;
> }
> state->reply_encpart.nonce = state->request->nonce;
>
> I think this change isn't necessary. Expanding from "REQ" to
> "REQUEST" doesn't do much for clarity, and it's referring to the name
> of a function anyway.
The general question here if the following substitutions are OK in the
general logs:
REQUEST vs REQ
REPLY vs REP
I agree that REQ and REP can stay for AS_REQ, AS_REP, TGS_REQ,
TGS_REP, KDC_REP or KDC_REQ as they are heavily used in Kerberos
protocol specs. In all other cases it would be preferable to have
"proper English" word, don't you agree.
>
>
> @@ -274,7 +274,7 @@ finish_process_as_req(struct as_req_state
> *state, krb5_error_code errcode)
> errcode = krb5_encrypt_tkt_part(kdc_context, &state-
> >server_keyblock,
> &state->ticket_reply);
> if (errcode) {
> - state->status = "ENCRYPTING_TICKET";
> + state->status = "ENCRYPT_TICKET";
> goto egress;
> }
> state->ticket_reply.enc_part.kvno = server_key->key_data_kvno;
>
> I think "ENCRYPT_TKT" would be better.
left unchanged
>
>
> @@ -283,7 +283,7 @@ finish_process_as_req(struct as_req_state
> *state, krb5_error_code errcode)
> &state->reply,
> state-
> >client_keyblock.enctype);
> if (errcode) {
> - state->status = "fast response handling";
> + state->status = "FAST_RESPONSE";
> goto egress;
> }
>
> I think FAST_RESPONSE_HANDLE_PADATA (to match the name of the
> function) or FAST_RESPONSE_PADATA would be better.
>
OK
>
> @@ -294,7 +294,7 @@ finish_process_as_req(struct as_req_state
> *state, krb5_error_code errcode)
> errcode = kdc_fast_handle_reply_key(state->rstate, &state-
> >client_keyblock,
> &as_encrypting_key);
> if (errcode) {
> - state->status = "generating reply key";
> + state->status = "GENERATE_REPLY_KEY";
> goto egress;
> }
> errcode = return_enc_padata(kdc_context, state->req_pkt, state-
> >request,
>
> I think FAST_HANDLE_REPLY_KEY (to match the name of the function) or
> FAST_REPLY_KEY would be better.
changed to "GENERATE_FAST_REPLY_KEY"
>
>
> @@ -314,7 +314,7 @@ finish_process_as_req(struct as_req_state
> *state, krb5_error_code errcode)
> if (client_key != NULL)
> state->reply.enc_part.kvno = client_key->key_data_kvno;
> if (errcode) {
> - state->status = "ENCODE_KDC_REP";
> + state->status = "ENCODE_KDC_REPLY";
> goto egress;
> }
>
> Like with FETCH_LAST_REQ, this status string should stay the same so
> that it will match the function name more closely.
>
reverted to ENCODE_KDC_REP
>
> @@ -475,20 +475,20 @@ process_as_req(krb5_kdc_req *request,
> krb5_data *req_pkt,
> return;
> }
> if (state->request->msg_type != KRB5_AS_REQ) {
> - state->status = "msg_type mismatch";
> + state->status = "VALIDATE_MESSAGE_TYPE";
> errcode = KRB5_BADMSGTYPE;
> goto errout;
> }
>
> I would prefer VALIDATE_MSG_TYPE here.
>
left unchanged
>
> if (fetch_asn1_field((unsigned char *) req_pkt->data,
> 1, 4, &encoded_req_body) != 0) {
> errcode = ASN1_BAD_ID;
> - state->status = "Finding req_body";
> + state->status = "CONSTRUCT_STATE";
> goto errout;
> }
>
> I don't understand why fetching the req_body should get a status
> string of "CONSTRUCT_STATE". Could you please clarify?
>
typo. Changed to FETCH_REQ_BODY
>
> errcode = kdc_find_fast(&state->request, &encoded_req_body,
> NULL, NULL,
> state->rstate, &state->inner_body);
> if (errcode) {
> - state->status = "error decoding FAST";
> + state->status = "DECODE_FAST";
> goto errout;
> }
> if (state->inner_body == NULL) {
>
> Why not FIND_FAST, which is closer to the function name?
OK
>
>
> @@ -496,7 +496,7 @@ process_as_req(krb5_kdc_req *request, krb5_data
> *req_pkt,
> errcode = krb5_copy_data(kdc_context, &encoded_req_body,
> &state->inner_body);
> if (errcode) {
> - state->status = "storing req body";
> + state->status = "STORE_REQ_BODY";
> goto errout;
> }
> }
>
> I think COPY_INNER_BODY would be better here.
>
left unchanged
>
> @@ -512,7 +512,7 @@ process_as_req(krb5_kdc_req *request, krb5_data
> *req_pkt,
> if ((errcode = krb5_unparse_name(kdc_context,
> state->request->client,
> &state->cname))) {
> - state->status = "UNPARSING_CLIENT";
> + state->status = "UNPARSE_CLIENT";
> goto errout;
> }
> limit_string(state->cname);
> @@ -571,7 +571,7 @@ process_as_req(krb5_kdc_req *request, krb5_data
> *req_pkt,
> if ((errcode = krb5_unparse_name(kdc_context,
> state->request->server,
> &state->sname))) {
> - state->status = "UNPARSING_SERVER";
> + state->status = "UNPARSE_SERVER";
> goto errout;
> }
> limit_string(state->sname);
> @@ -623,7 +623,7 @@ process_as_req(krb5_kdc_req *request, krb5_data
> *req_pkt,
>
> if ((errcode = krb5_c_make_random_key(kdc_context, useenctype,
> &state->session_key))) {
> - state->status = "RANDOM_KEY_FAILED";
> + state->status = "MAKE_RANDOM_KEY";
> goto errout;
> }
>
> The above three seem OK.
>
> @@ -707,8 +707,8 @@ process_as_req(krb5_kdc_req *request, krb5_data
> *req_pkt,
> state->request->client,
>
> krb5_anonymous_principal())) {
> errcode = KRB5KDC_ERR_BADOPTION;
> - state->status = "Anonymous requested but anonymous "
> - "principal not used.";
> + /* Anonymous requested but anonymous principal not
> used.*/
> + state->status = "VALIDATE_ANONYMOUS";
> goto errout;
> }
> setflag(state->enc_tkt_reply.flags, TKT_FLG_ANONYMOUS);
>
> I think we should find something more specific than
> VALIDATE_ANONYMOUS, because the error text associated with
> KRB5KDC_ERR_BADOPTION is simply "KDC can't fulfill requested option".
> Maybe it should be ANONYMOUS_OPTION_WRONG_PRINC or
> VALIDATE_ANONYMOUS_PRINC.
>
changed to VALIDATE_ANONYMOUS_PRINCIPAL
>
> @@ -717,7 +717,7 @@ process_as_req(krb5_kdc_req *request, krb5_data
> *req_pkt,
> errcode = krb5_copy_principal(kdc_context,
> krb5_anonymous_principal(),
> &state->request->client);
> if (errcode) {
> - state->status = "Copying anonymous principal";
> + state->status = "COPY_ANONYMOUS";
> goto errout;
> }
> state->enc_tkt_reply.client = state->request->client;
>
>
>
> diff --git a/src/kdc/do_tgs_req.c b/src/kdc/do_tgs_req.c
> index ae5e757..4b240b1 100644
> --- a/src/kdc/do_tgs_req.c
> +++ b/src/kdc/do_tgs_req.c
> @@ -177,7 +177,7 @@ process_tgs_req(struct server_handle *handle,
> krb5_data *pkt,
>
> if (!header_ticket) {
> errcode = KRB5_NO_TKT_SUPPLIED; /* XXX? */
> - status="UNEXPECTED NULL in header_ticket";
> + status="VALIDATE_HEADER_TICKET";
> goto cleanup;
> }
> scratch.length = pa_tgs_req->length;
>
> This is not really accurate, because kdc_process_tgs_req() already
> validates header_ticket. The value of errcode is also incorrect,
> because it's not in the protocol error code number space. I think
> this might be an "impossible" error path, but I haven't checked
> thoroughly.
left unchanged for now
>
>
> @@ -185,7 +185,7 @@ process_tgs_req(struct server_handle *handle,
> krb5_data *pkt,
> errcode = kdc_find_fast(&request, &scratch, subkey,
> header_ticket->enc_part2->session,
> state, NULL);
> if (errcode !=0) {
> - status = "kdc_find_fast";
> + status = "DECODE_FAST";
> goto cleanup;
> }
>
> As with the do_as_req.c case, this should be FIND_FAST.
OK
>
>
> @@ -580,7 +580,7 @@ process_tgs_req(struct server_handle *handle,
> krb5_data *pkt,
> /* assemble new transited field into allocated storage */
> if (header_enc_tkt->transited.tr_type !=
> KRB5_DOMAIN_X500_COMPRESS) {
> - status = "BAD_TRTYPE";
> + status = "VALIDATE_TRANSIT_TYPE";
> errcode = KRB5KDC_ERR_TRTYPE_NOSUPP;
> goto cleanup;
> }
>
> I think VALIDATE_TR_TYPE is better here.
left unchanged. "TR" is not a very intuitive acronym for "transited"
>
>
> @@ -596,7 +596,7 @@ process_tgs_req(struct server_handle *handle,
> krb5_data *pkt,
> header_ticket->server,
> enc_tkt_reply.client,
> request->server))) {
> - status = "ADD_TR_FAIL";
> + status = "UPDATE_TRANSIT_LIST";
> goto cleanup;
> }
> newtransited = 1;
>
> I think this should be ADD_TO_TRANSITED, to match the name of the
> function that would produce this error condition.
changed to ADD_TO_TRANSITED_LIST
>
>
>
> @@ -663,7 +663,7 @@ process_tgs_req(struct server_handle *handle,
> krb5_data *pkt,
> if (!isflagset(request->kdc_options, KDC_OPT_ENC_TKT_IN_SKEY))
> krb5_free_keyblock_contents(kdc_context, &encrypting_key);
> if (errcode) {
> - status = "TKT_ENCRYPT";
> + status = "ENCRYPT_TICKET";
> goto cleanup;
> }
> ticket_reply.enc_part.kvno = ticket_kvno;
>
> As with do_as_req.c, this should be ENCRYPT_TKT.
left unchanged.
>
>
> @@ -679,7 +679,7 @@ process_tgs_req(struct server_handle *handle,
> krb5_data *pkt,
> &reply,
> &reply_encpart);
> if (errcode) {
> - status = "KDC_RETURN_S4U2SELF_PADATA";
> + status = "FETCH_S4U2SELF_PADATA";
> goto cleanup;
> }
> }
>
> This should be MAKE_S4U2SELF_REP to better match the function that
> would produce this error condition.
left unchanged.
>
>
> @@ -715,13 +715,13 @@ process_tgs_req(struct server_handle *handle,
> krb5_data *pkt,
> errcode = kdc_fast_response_handle_padata(state, request, &reply,
> subkey ? subkey-
> >enctype : header_ticket->enc_part2->session->enctype);
> if (errcode !=0 ) {
> - status = "Preparing FAST padata";
> + status = "HANDLE_FAST_RESPONSE";
> goto cleanup;
> }
>
> As with do_as_req.c, I think FAST_RESPONSE_HANDLE_PADATA (to match the
> name of the function) or FAST_RESPONSE_PADATA would be better.
>
changed to HANDLE_FAST_RESPONSE_PADATA
>
> errcode =kdc_fast_handle_reply_key(state,
> subkey?subkey:header_ticket-
> >enc_part2->session, &reply_key);
> if (errcode) {
> - status = "generating reply key";
> + status = "GENERATE_REPLY_KEY";
> goto cleanup;
> }
> errcode = return_enc_padata(kdc_context, pkt, request,
>
> As with do_as_req.c, I think FAST_HANDLE_REPLY_KEY (to match the name
> of the function) or FAST_REPLY_KEY would be better.
changed to HANDLE_FAST_REPLY_KEY
>
>
>
> @@ -741,7 +741,7 @@ process_tgs_req(struct server_handle *handle,
> krb5_data *pkt,
> reply_key,
> &reply, response);
> if (errcode) {
> - status = "ENCODE_KDC_REP";
> + status = "ENCODE_KDC_REPLY";
> } else {
> status = "ISSUE";
> }
>
> As with do_as_req.c, this should remain ENCODE_KDC_REP.
reverted to the original string
>
>
> @@ -989,7 +989,7 @@ gen_session_key(kdc_realm_t *kdc_active_realm,
> krb5_kdc_req *req,
> retval = krb5_c_make_random_key(kdc_context, useenctype, skey);
> if (retval != 0) {
> /* random key failed */
> - *status = "RANDOM_KEY_FAILED";
> + *status = "MAKE_RANDOM_KEY";
> goto cleanup;
> }
> cleanup:
>
> This seems OK.
Zhanna Tsitkov
tsitkova at mit.edu
More information about the krbdev
mailing list