KDC status strings consistency
Tom Yu
tlyu at MIT.EDU
Mon Sep 30 17:37:51 EDT 2013
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.
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.
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.
@@ -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.
@@ -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.
@@ -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.
@@ -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.
@@ -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.
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?
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?
@@ -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.
@@ -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.
@@ -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.
@@ -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.
@@ -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.
@@ -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.
@@ -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.
@@ -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.
@@ -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.
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.
@@ -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.
@@ -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.
More information about the krbdev
mailing list