PR#35: Fixes around credential handling - gssproxy

gssproxy

#35 Fixes around credential handling

Closed 7 years ago by simo. Opened 7 years ago by simo.

simo/gssproxy cred_fixes into master

Always initialize ccache when storing.

Simo Sorce • 7 years ago

7f3ab1c

In acquire_cred_from, probe for remote creds

Simo Sorce • 7 years ago

4525723

Make sure to pass on request for delegated creds

Simo Sorce • 7 years ago

126bd3a

Add cred_store support for local calls.

Simo Sorce • 7 years ago

5cbecfe

proxy/src/client/gpm_accept_sec_context.c

file modified

		`@@ -50,6 +50,11 @@`
		`}`
		`}`

		`+ /* check if we want delegated creds */`
		`+ if (delegated_cred_handle) {`
		`+ arg->ret_deleg_cred = true;`
		`+ }`
		`+`
		`/* execute proxy request */`
		`ret = gpm_make_call(GSSX_ACCEPT_SEC_CONTEXT, &uarg, &ures);`
		`if (ret) {`

proxy/src/mechglue/gpp_acquire_cred.c

file modified

+85 -22

		`@@ -8,6 +8,7 @@`
		`OM_uint32 time_req,`
		`const gss_OID_set desired_mechs,`
		`gss_cred_usage_t cred_usage,`
		`+ gss_const_key_value_set_t cred_store,`
		`struct gpp_cred_handle *out_cred_handle,`
		`gss_OID_set *actual_mechs,`
		`OM_uint32 *time_rec)`
		`@@ -43,14 +44,15 @@`
		`goto done;`
		`}`

		`- maj = gss_acquire_cred(&min,`
		`- name ? name->local : NULL,`
		`- time_req,`
		`- special_mechs,`
		`- cred_usage,`
		`- &out_cred_handle->local,`
		`- actual_mechs,`
		`- time_rec);`
		`+ maj = gss_acquire_cred_from(&min,`
		`+ name ? name->local : NULL,`
		`+ time_req,`
		`+ special_mechs,`
		`+ cred_usage,`
		`+ cred_store,`
		`+ &out_cred_handle->local,`
		`+ actual_mechs,`
		`+ time_rec);`

		`done:`
		`*minor_status = min;`
		`@@ -67,9 +69,25 @@`
		`gss_OID_set *actual_mechs,`
		`OM_uint32 *time_rec)`
		`{`
		`+ return gssi_acquire_cred_from(minor_status, desired_name, time_req,`
		`+ desired_mechs, cred_usage, NULL,`
		`+ output_cred_handle, actual_mechs, time_rec);`
		`+ }`
		`+`
		`+ OM_uint32 gssi_acquire_cred_from(OM_uint32 *minor_status,`
		`+ const gss_name_t desired_name,`
		`+ OM_uint32 time_req,`
		`+ const gss_OID_set desired_mechs,`
		`+ gss_cred_usage_t cred_usage,`
		`+ gss_const_key_value_set_t cred_store,`
		`+ gss_cred_id_t *output_cred_handle,`
		`+ gss_OID_set *actual_mechs,`
		`+ OM_uint32 *time_rec)`
		`+ {`
		`enum gpp_behavior behavior;`
		`struct gpp_name_handle *name;`
		`struct gpp_cred_handle *out_cred_handle = NULL;`
		`+ struct gssx_cred *in_cred_remote = NULL;`
		`OM_uint32 maj, min;`
		`OM_uint32 tmaj, tmin;`

		`@@ -93,11 +111,38 @@`
		`name = (struct gpp_name_handle *)desired_name;`
		`behavior = gpp_get_behavior();`

		`+ /* if a cred_store option is passed in, check if it references`
		`+ * valid credentials, if so switch behavior appropriately */`
		`+ if (cred_store) {`
		`+ for (unsigned i = 0; i < cred_store->count; i++) {`
		`+ if (strcmp(cred_store->elements[i].key, "ccache") == 0) {`
		`+ gssx_cred remote = {0};`
		`+ maj = gppint_retrieve_remote_creds(&min,`
		`+ cred_store->elements[i].value, NULL, &remote);`
		`+ if (maj == GSS_S_COMPLETE) {`
		`+ in_cred_remote = malloc(sizeof(gssx_cred));`
		`+ if (!in_cred_remote) {`
		`+ maj = GSS_S_FAILURE;`
		`+ min = ENOMEM;`
		`+ goto done;`
		`+ }`
		`+ *in_cred_remote = remote;`
		`+ break;`
		`+ }`
		`+ }`
		`+ }`
		`+ if (in_cred_remote) {`
		`+ behavior = GPP_REMOTE_ONLY;`
		`+ } else {`
		`+ behavior = GPP_LOCAL_ONLY;`
		`+ }`
		`+ }`
		`+`
		`/* See if we should try local first */`
		`if (behavior == GPP_LOCAL_ONLY \|\| behavior == GPP_LOCAL_FIRST) {`

		`maj = acquire_local(&min, NULL, name,`
		`- time_req, desired_mechs, cred_usage,`
		`+ time_req, desired_mechs, cred_usage, cred_store,`
		`out_cred_handle, actual_mechs, time_rec);`

		`if (maj == GSS_S_COMPLETE \|\| behavior == GPP_LOCAL_ONLY) {`
		`@@ -117,7 +162,7 @@`
		`}`
		`}`

		`- maj = gpm_acquire_cred(&min, NULL,`
		`+ maj = gpm_acquire_cred(&min, in_cred_remote,`
		`name ? name->remote : NULL,`
		`time_req,`
		`desired_mechs,`
		`@@ -132,7 +177,7 @@`
		`if (behavior == GPP_REMOTE_FIRST) {`
		`/* So remote failed, but we can fallback to local, try that */`
		`maj = acquire_local(&min, NULL, name,`
		`- time_req, desired_mechs, cred_usage,`
		`+ time_req, desired_mechs, cred_usage, cred_store,`
		`out_cred_handle, actual_mechs, time_rec);`
		`}`

		`@@ -143,6 +188,10 @@`
		`maj = tmaj;`
		`min = tmin;`
		`}`
		`+ if (in_cred_remote) {`
		`+ xdr_free((xdrproc_t)xdr_gssx_cred, (char *)in_cred_remote);`
		`+ free(in_cred_remote);`
		`+ }`
		`if (maj == GSS_S_COMPLETE) {`
		`*output_cred_handle = (gss_cred_id_t)out_cred_handle;`
		`} else {`
		`@@ -164,6 +213,26 @@`
		`OM_uint32 *initiator_time_rec,`
		`OM_uint32 *acceptor_time_rec)`
		`{`
		`+ return gssi_add_cred_from(minor_status, input_cred_handle, desired_name,`
		`+ desired_mech, cred_usage, initiator_time_req,`
		`+ acceptor_time_req, NULL, output_cred_handle,`
		`+ actual_mechs, initiator_time_rec,`
		`+ acceptor_time_rec);`
		`+ }`
		`+`
		`+ OM_uint32 gssi_add_cred_from(OM_uint32 *minor_status,`
		`+ const gss_cred_id_t input_cred_handle,`
		`+ const gss_name_t desired_name,`
		`+ const gss_OID desired_mech,`
		`+ gss_cred_usage_t cred_usage,`
		`+ OM_uint32 initiator_time_req,`
		`+ OM_uint32 acceptor_time_req,`
		`+ gss_const_key_value_set_t cred_store,`
		`+ gss_cred_id_t *output_cred_handle,`
		`+ gss_OID_set *actual_mechs,`
		`+ OM_uint32 *initiator_time_rec,`
		`+ OM_uint32 *acceptor_time_rec)`
		`+ {`
		`gss_OID_set desired_mechs = GSS_C_NO_OID_SET;`
		`OM_uint32 time_req, time_rec;`
		`OM_uint32 maj, min;`
		`@@ -206,14 +275,9 @@`
		`time_req = 0;`
		`}`

		`- maj = gssi_acquire_cred(minor_status,`
		`- desired_name,`
		`- time_req,`
		`- desired_mechs,`
		`- cred_usage,`
		`- output_cred_handle,`
		`- actual_mechs,`
		`- &time_rec);`
		`+ maj = gssi_acquire_cred_from(minor_status, desired_name, time_req,`
		`+ desired_mechs, cred_usage, NULL,`
		`+ output_cred_handle, actual_mechs, &time_rec);`
		`if (maj == GSS_S_COMPLETE) {`
		`if (acceptor_time_rec &&`
		`(cred_usage == GSS_C_ACCEPT \|\| cred_usage == GSS_C_BOTH)) {`
		`@@ -375,7 +439,7 @@`
		`if (behavior == GPP_LOCAL_ONLY \|\| behavior == GPP_LOCAL_FIRST) {`

		`maj = acquire_local(&min, impersonator_cred_handle, name,`
		`- time_req, desired_mechs, cred_usage,`
		`+ time_req, desired_mechs, cred_usage, NULL,`
		`out_cred_handle, actual_mechs, time_rec);`

		`if (maj == GSS_S_COMPLETE \|\| behavior == GPP_LOCAL_ONLY) {`
		`@@ -412,7 +476,7 @@`
		`if (behavior == GPP_REMOTE_FIRST) {`
		`/* So remote failed, but we can fallback to local, try that */`
		`maj = acquire_local(&min, impersonator_cred_handle, name,`
		`- time_req, desired_mechs, cred_usage,`
		`+ time_req, desired_mechs, cred_usage, NULL,`
		`out_cred_handle, actual_mechs, time_rec);`
		`}`

		`@@ -431,4 +495,3 @@`
		`*minor_status = gpp_map_error(min);`
		`return maj;`
		`}`
		`-`

proxy/src/mechglue/gpp_creds.c

file modified

+51 -20

		`@@ -6,7 +6,9 @@`
		`#define GPKRB_SRV_NAME "Encrypted/Credentials/v1@X-GSSPROXY:"`
		`#define GPKRB_MAX_CRED_SIZE 1024 * 512`

		`- uint32_t gpp_store_remote_creds(uint32_t min, gssx_cred creds)`
		`+ uint32_t gpp_store_remote_creds(uint32_t *min,`
		`+ gss_const_key_value_set_t cred_store,`
		`+ gssx_cred *creds)`
		`{`
		`krb5_context ctx = NULL;`
		`krb5_ccache ccache = NULL;`
		`@@ -24,8 +26,20 @@`
		`ret = krb5_init_context(&ctx);`
		`if (ret) return ret;`

		`- ret = krb5_cc_default(ctx, &ccache);`
		`- if (ret) goto done;`
		`+ if (cred_store) {`
		`+ for (unsigned i = 0; i < cred_store->count; i++) {`
		`+ if (strcmp(cred_store->elements[i].key, "ccache") == 0) {`
		`+ ret = krb5_cc_resolve(ctx, cred_store->elements[i].value,`
		`+ &ccache);`
		`+ if (ret) goto done;`
		`+ break;`
		`+ }`
		`+ }`
		`+ }`
		`+ if (!ccache) {`
		`+ ret = krb5_cc_default(ctx, &ccache);`
		`+ if (ret) goto done;`
		`+ }`

		`ret = krb5_parse_name(ctx,`
		`creds->desired_name.display_name.octet_string_val,`
		`@@ -44,14 +58,10 @@`
		`}`
		`cred.ticket.length = xdr_getpos(&xdrctx);`

		`- ret = krb5_cc_store_cred(ctx, ccache, &cred);`
		`-`
		`- if (ret == KRB5_FCC_NOFILE) {`
		`- /* If a ccache does not exit, try to create one */`
		`- ret = krb5_cc_initialize(ctx, ccache, cred.client);`
		`- if (ret) goto done;`
		`-`
		`- /* and try again to store the cred */`
		`+ /* Always initialize and destroy any existing contents to avoid pileup of`
		`+ * entries */`
		`+ ret = krb5_cc_initialize(ctx, ccache, cred.client);`
		`+ if (ret == 0) {`
		`ret = krb5_cc_store_cred(ctx, ccache, &cred);`
		`}`

		`@@ -65,8 +75,8 @@`
		`return ret ? GSS_S_FAILURE : GSS_S_COMPLETE;`
		`}`

		`- static uint32_t retrieve_remote_creds(uint32_t min, gssx_name name,`
		`- gssx_cred *creds)`
		`+ OM_uint32 gppint_retrieve_remote_creds(uint32_t min, const char ccache_name,`
		`+ gssx_name name, gssx_cred creds)`
		`{`
		`krb5_context ctx = NULL;`
		`krb5_ccache ccache = NULL;`
		`@@ -82,7 +92,11 @@`
		`ret = krb5_init_context(&ctx);`
		`if (ret) goto done;`

		`- ret = krb5_cc_default(ctx, &ccache);`
		`+ if (ccache_name) {`
		`+ ret = krb5_cc_resolve(ctx, ccache_name, &ccache);`
		`+ } else {`
		`+ ret = krb5_cc_default(ctx, &ccache);`
		`+ }`
		`if (ret) goto done;`

		`if (name) {`
		`@@ -189,7 +203,9 @@`
		`memset(&remote, 0, sizeof(gssx_cred));`

		`/* We intentionally ignore failures as finding creds is optional */`
		`- maj = retrieve_remote_creds(&min, name ? name->remote : NULL, &remote);`
		`+ maj = gppint_retrieve_remote_creds(&min, NULL,`
		`+ name ? name->remote : NULL,`
		`+ &remote);`
		`if (maj == GSS_S_COMPLETE) {`
		`premote = &remote;`
		`}`
		`@@ -497,6 +513,21 @@`
		`gss_OID_set *elements_stored,`
		`gss_cred_usage_t *cred_usage_stored)`
		`{`
		`+ return gssi_store_cred_into(minor_status, input_cred_handle, input_usage,`
		`+ desired_mech, overwrite_cred, default_cred,`
		`+ NULL, elements_stored, cred_usage_stored);`
		`+ }`
		`+`
		`+ OM_uint32 gssi_store_cred_into(OM_uint32 *minor_status,`
		`+ const gss_cred_id_t input_cred_handle,`
		`+ gss_cred_usage_t input_usage,`
		`+ const gss_OID desired_mech,`
		`+ OM_uint32 overwrite_cred,`
		`+ OM_uint32 default_cred,`
		`+ gss_const_key_value_set_t cred_store,`
		`+ gss_OID_set *elements_stored,`
		`+ gss_cred_usage_t *cred_usage_stored)`
		`+ {`
		`struct gpp_cred_handle *cred = NULL;`
		`OM_uint32 maj, min;`

		`@@ -509,14 +540,14 @@`
		`cred = (struct gpp_cred_handle *)input_cred_handle;`

		`if (cred->remote) {`
		`- maj = gpp_store_remote_creds(&min, cred->remote);`
		`+ maj = gpp_store_remote_creds(&min, cred_store, cred->remote);`
		`goto done;`
		`}`

		`- maj = gss_store_cred(&min, cred->local, input_usage,`
		`- gpp_special_mech(desired_mech),`
		`- overwrite_cred, default_cred,`
		`- elements_stored, cred_usage_stored);`
		`+ maj = gss_store_cred_into(&min, cred->local, input_usage,`
		`+ gpp_special_mech(desired_mech),`
		`+ overwrite_cred, default_cred, cred_store,`
		`+ elements_stored, cred_usage_stored);`
		`done:`
		`*minor_status = gpp_map_error(min);`
		`return maj;`

proxy/src/mechglue/gss_plugin.h

file modified

+36

		`@@ -81,6 +81,16 @@`
		`gss_OID_set *actual_mechs,`
		`OM_uint32 *time_rec);`

		`+ OM_uint32 gssi_acquire_cred_from(OM_uint32 *minor_status,`
		`+ const gss_name_t desired_name,`
		`+ OM_uint32 time_req,`
		`+ const gss_OID_set desired_mechs,`
		`+ gss_cred_usage_t cred_usage,`
		`+ gss_const_key_value_set_t cred_store,`
		`+ gss_cred_id_t *output_cred_handle,`
		`+ gss_OID_set *actual_mechs,`
		`+ OM_uint32 *time_rec);`
		`+`
		`OM_uint32 gssi_add_cred(OM_uint32 *minor_status,`
		`const gss_cred_id_t input_cred_handle,`
		`const gss_name_t desired_name,`
		`@@ -93,6 +103,19 @@`
		`OM_uint32 *initiator_time_rec,`
		`OM_uint32 *acceptor_time_rec);`

		`+ OM_uint32 gssi_add_cred_from(OM_uint32 *minor_status,`
		`+ const gss_cred_id_t input_cred_handle,`
		`+ const gss_name_t desired_name,`
		`+ const gss_OID desired_mech,`
		`+ gss_cred_usage_t cred_usage,`
		`+ OM_uint32 initiator_time_req,`
		`+ OM_uint32 acceptor_time_req,`
		`+ gss_const_key_value_set_t cred_store,`
		`+ gss_cred_id_t *output_cred_handle,`
		`+ gss_OID_set *actual_mechs,`
		`+ OM_uint32 *initiator_time_rec,`
		`+ OM_uint32 *acceptor_time_rec);`
		`+`
		`OM_uint32 gssi_acquire_cred_with_password(OM_uint32 *minor_status,`
		`const gss_name_t desired_name,`
		`const gss_buffer_t password,`
		`@@ -113,6 +136,9 @@`
		`gss_OID_set *actual_mechs,`
		`OM_uint32 *time_rec);`

		`+ OM_uint32 gppint_retrieve_remote_creds(uint32_t min, const char ccache_name,`
		`+ gssx_name name, gssx_cred creds);`
		`+`
		`OM_uint32 gppint_get_def_creds(OM_uint32 *minor_status,`
		`enum gpp_behavior behavior,`
		`struct gpp_name_handle *name,`
		`@@ -153,6 +179,16 @@`
		`gss_OID_set *elements_stored,`
		`gss_cred_usage_t *cred_usage_stored);`

		`+ OM_uint32 gssi_store_cred_into(OM_uint32 *minor_status,`
		`+ const gss_cred_id_t input_cred_handle,`
		`+ gss_cred_usage_t input_usage,`
		`+ const gss_OID desired_mech,`
		`+ OM_uint32 overwrite_cred,`
		`+ OM_uint32 default_cred,`
		`+ gss_const_key_value_set_t cred_store,`
		`+ gss_OID_set *elements_stored,`
		`+ gss_cred_usage_t *cred_usage_stored);`
		`+`
		`OM_uint32 gssi_release_cred(OM_uint32 *minor_status,`
		`gss_cred_id_t *cred_handle);`

simo commented 7 years ago

Allow client to proprerly handle cred store for local calls when interposed, still no cred store transmission to proxy
Allow client to properly handle delegated credentials on interposed accept call

simo commented 7 years ago

@rharwood can you check this ?

rebased

7 years ago

simo commented 7 years ago

Updated with more fixes

rharwood commented 7 years ago

In general this looks good to me. Before merging, I think it would be good if we had a test case for this. (Doesn't need to be anything major; I can write it if you want.)

Since this is local-only with no transmission to the proxy, there shouldn't be any issues with leakage that we need to worry about.

4 new commits added

Always initialize ccache when storing.
In acquire_cred_from, probe for remote creds
Make sure to pass on request for delegated creds
Add cred_store support for local calls.

7 years ago

simo commented 7 years ago

If you could add a test it would be really appreciated.
I just fixed another 2 issues I was having with the previous set.
One crash and another came up as it took me enough time analyzing the crash, that when I resolved it I found stale entries in the ccaches. So I added a nother patch to the pile. They are all connected.

1 new commit added