After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 659680 - Krb5 key hashing support
Krb5 key hashing support
Status: RESOLVED INCOMPLETE
Product: libgnome-keyring
Classification: Core
Component: General
unspecified
Other Linux
: Normal normal
: ---
Assigned To: GNOME keyring maintainer(s)
GNOME keyring maintainer(s)
: 526045 (view as bug list)
Depends on:
Blocks: 567701
 
 
Reported: 2011-09-21 09:47 UTC by Pavel Ocheretny
Modified: 2019-02-22 11:46 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Code for key hashing (13.74 KB, patch)
2011-09-21 09:47 UTC, Pavel Ocheretny
needs-work Details | Review
krb5 patch V2 (13.01 KB, patch)
2011-09-22 09:52 UTC, Or Goshen
needs-work Details | Review
krb5 patch v3 (13.62 KB, patch)
2011-09-25 12:17 UTC, Or Goshen
none Details | Review
krb5 patch V4 (13.62 KB, patch)
2011-09-25 13:32 UTC, Or Goshen
none Details | Review
krb5 patch V5 (14.36 KB, patch)
2011-09-27 09:29 UTC, Or Goshen
none Details | Review
Related documentation (2.20 KB, patch)
2011-10-03 08:41 UTC, Or Goshen
none Details | Review
I've updated the patch with a bit of cleanup. In particular: (16.40 KB, patch)
2011-10-03 19:00 UTC, Stef Walter
none Details | Review
Updated the patch to work with git master. (16.86 KB, patch)
2012-09-19 08:26 UTC, Stef Walter
none Details | Review

Description Pavel Ocheretny 2011-09-21 09:47:10 UTC
Created attachment 197133 [details] [review]
Code for key hashing

gnome-keyring isnt suppose to do any "talking" to the KDC by itself.
The following code add support for an interface with a single method that accepts an encryption type + salt and return the hashed key.
Comment 1 Stef Walter 2011-09-21 09:51:44 UTC
Comment on attachment 197133 [details] [review]
Code for key hashing

Changing the type so I can review.
Comment 2 Stef Walter 2011-09-21 15:34:36 UTC
Review of attachment 197133 [details] [review]:

This looks nice. I'm glad you've got the details and methodology hashed out. Some initial review...

First of all this patch does not apply to git master. For this reason I haven't tested it. Are there other commits that come between this patch and gnome-keyring git master? If so, then those patches should be added to other bug(s), and this bug made to depend on it/them.

::: configure.ac
@@ +255,3 @@
+# We need exactly one of MIT and Heimdal Kerberos,
+# and we care which one we're asked to use.
+	  	  	AC_ARG_WITH(mitkrb5, [ --with-mitkrb5 Compile against MIT Kerberos

This dependency should be optional. gnome-keyring should continue to with and build without this support.

If this support is not compiled in then the relevant dbus interfaces would not be present.

We should also show the status of whether the kerberos support is built in, at the end of the configure output. You can see the various echo lines near the end of configure.ac

::: daemon/dbus/gkd-secret-introspect.c
@@ +97,3 @@
 	"\n"
+	"	<interface name='org.gnome.keyring.Credential.Kerberos'>\n"
+	"		<method name='GetKey'>\n"

Should this be called 'HashKey' or something more descriptive of the action performed? However if 'GetKey' is the normal nomenclature for this in kerberos, then I'm certainly fine with keeping it that way here.

::: daemon/dbus/gkd-secret-kerberos.c
@@ +41,3 @@
+
+static gpointer
+master_secret_for_collection (GkdSecretService *service, GckObject *collection,

Will an identical function also be present in the NTLM code? How about moving this function to gkd-secret-objects.c and exposing it so both the NTLM and Kerberos code can call it?

@@ +78,3 @@
+method_get_key (GkdSecretService *service, GckObject *object,
+                       RetrieveSecretCb retrieve_secret_cb, DBusMessage *message)
+{

For new code, we're putting function arguments on multiple lines. I know all the old code isn't perfectly consistent. See HACKING for details.

Can you fix this in all the function declarations/definitions of the patch?

@@ +96,3 @@
+	if (!dbus_message_get_args (message, NULL,
+	                            DBUS_TYPE_INT32, &enctype,
+				   DBUS_TYPE_ARRAY, DBUS_TYPE_BYTE, &salt_data.data, &salt_data.length,

I know it's a nitpick, but could you fix the various indentation issues? We use tabs to indent lines, and then when wrapping lines, we use spaces to align the wrapped lines at the appropriate place. See HACKING for more details.

@@ +110,3 @@
+		reply = dbus_message_new_error_printf (message, DBUS_ERROR_FAILED,
+		                                       "No secret available for credentials: %s",
+		                                       egg_error_message (error));

I don't think error is set here.

@@ +117,3 @@
+	password_data.length = n_secret;
+	
+	retval = krb5_init_context(&context);

What kind of IO does this function do? Can it block?

@@ +120,3 @@
+	if (retval) {
+		reply = dbus_message_new_error_printf (message, DBUS_ERROR_FAILED,
+		                                       "Could not init kerberos context");

Can krb5_init_context() fail due to exernal circumstances (missing config, etc), or only due to programmer error? We only return dbus errors due to programmer error.

@@ +127,3 @@
+	if (retval) {
+		reply = dbus_message_new_error_printf (message, DBUS_ERROR_FAILED,
+		                                       "Could not hash key");

Under what circumstances can krb5_c_string_to_key() fail? Is it only invalid input? Should the error message reflect the fact that invalid input was provided?

@@ +136,3 @@
+	                          DBUS_TYPE_INVALID);
+
+	krb5_free_keyblock_contents(NULL, &key);

Please leave a space between all functions and their arguments.

@@ +139,3 @@
+
+error_lvl3:
+	krb5_free_context(context);

Instead of using multiple error levels like this, could we use one 'done:' label? Both egg_secure_free() and free() accept NULL pointers, so by setting the relevant variables to a NULL pointer at the top of the function, we can run all three of these cleanup calls without issue.

@@ +143,3 @@
+	egg_secure_free (secret);
+error_lvl1:
+	free(salt_data.data);

Are you sure we should be using free() here? Isn't salt_data.data owned by the message?

::: daemon/dbus/gkd-secret-objects.c
@@ +814,3 @@
+	else if (dbus_message_has_interface (message, CREDENTIAL_KERBEROS_INTERFACE))
+		return gkd_secret_kerberos_dispatch_for_collection (self->service, object, message);
+

I haven't compiled this (since the patch doesn't apply). But isn't there a missing header for this function?
Comment 3 Stef Walter 2011-09-21 16:09:53 UTC
*** Bug 526045 has been marked as a duplicate of this bug. ***
Comment 4 Or Goshen 2011-09-22 09:52:26 UTC
Created attachment 197222 [details] [review]
krb5 patch V2
Comment 5 Stef Walter 2011-09-22 19:44:43 UTC
Review of attachment 197222 [details] [review]:

Thanks for making those tweaks. Shaping up nicely. However:

I get this error when I try to configure gnome-keyring. This is both with and without a --with-krb5=heimdal argument. Therefore I couldn't go ahead with any.

daemon/dbus/Makefile.am:36: WITH_KERB5 does not appear in AM_CONDITIONAL

Which makes me wonder, how do I test this in any case? Could you attach a python script or the source for a simple client side script to this bug? Or is there another easy reproducible method for testing this. If a test environment (config files etc) is needed, could you document what it takes to setup an appropriate test environment. You could put that documentation here:

http://live.gnome.org/GnomeKeyring/Kerberos

I've also made a few other nitpicks, that would be nice to fix before this goes in.

::: configure.ac
@@ +229,3 @@
+if test $krb5type != no; then
+# Default args required by both MIT Kerberos and Heimdal Kerberos
+              [enable experimental support for Kerberos v5 (Using either MIT Kerberos v5 or Heimdal)])],

Are these flags only used in gnome-keyring? If so, then could you use AC_DEFINE so they get added to config.h instead?

In addition could you use WITH_KERB5 instead of USE_KRB5? If you look through the sources, that's the general form we've been using when dealing with optional components.

::: daemon/dbus/gkd-secret-kerberos.c
@@ +39,3 @@
+static DBusMessage*
+method_get_key (GkdSecretService *service, GckObject *object,
+		RetrieveSecretCb retrieve_secret_cb, DBusMessage *message)

As noted earlier, could you wrap one argument per line in function declarations?

::: daemon/dbus/gkd-secret-kerberos.h
@@ +31,3 @@
+DBusMessage*        gkd_secret_kerberos_dispatch_for_collection      (GkdSecretService *service,
+                                                                  GckObject *collection,
+                                                                  DBusMessage *message);

Could you align the arguments? Thanks!

::: daemon/dbus/gkd-secret-objects.c
@@ +59,3 @@
+gpointer
+master_secret_for_collection (GkdSecretService *service, GckObject *collection,
+                              gsize *n_secret, GError **error)

As noted earlier, in new code function definitions please wrap one argument per line. Thanks!

::: daemon/dbus/gkd-secret-objects.h
@@ +44,3 @@
+gpointer
+master_secret_for_collection (GkdSecretService *service, GckObject *collection,
+                              gsize *n_secret, GError **error);

When we make a function public, we generally name it with a prefix to help developers easily know what functionality and file it belongs to.

Could you prefix this appropriately similar to the other functions in this file.

Could you also wrap it and place it along side the other functions declarations in this file?

Thank you.
Comment 6 Stef Walter 2011-09-22 19:44:45 UTC
Review of attachment 197222 [details] [review]:

Thanks for making those tweaks. Shaping up nicely. However:

I get this error when I try to configure gnome-keyring. This is both with and without a --with-krb5=heimdal argument. Therefore I couldn't go ahead with any.

daemon/dbus/Makefile.am:36: WITH_KERB5 does not appear in AM_CONDITIONAL

Which makes me wonder, how do I test this in any case? Could you attach a python script or the source for a simple client side script to this bug? Or is there another easy reproducible method for testing this. If a test environment (config files etc) is needed, could you document what it takes to setup an appropriate test environment. You could put that documentation here:

http://live.gnome.org/GnomeKeyring/Kerberos

I've also made a few other nitpicks, that would be nice to fix before this goes in.

::: configure.ac
@@ +229,3 @@
+if test $krb5type != no; then
+# Default args required by both MIT Kerberos and Heimdal Kerberos
+              [enable experimental support for Kerberos v5 (Using either MIT Kerberos v5 or Heimdal)])],

Are these flags only used in gnome-keyring? If so, then could you use AC_DEFINE so they get added to config.h instead?

In addition could you use WITH_KERB5 instead of USE_KRB5? If you look through the sources, that's the general form we've been using when dealing with optional components.

::: daemon/dbus/gkd-secret-kerberos.c
@@ +39,3 @@
+static DBusMessage*
+method_get_key (GkdSecretService *service, GckObject *object,
+		RetrieveSecretCb retrieve_secret_cb, DBusMessage *message)

As noted earlier, could you wrap one argument per line in function declarations?

::: daemon/dbus/gkd-secret-kerberos.h
@@ +31,3 @@
+DBusMessage*        gkd_secret_kerberos_dispatch_for_collection      (GkdSecretService *service,
+                                                                  GckObject *collection,
+                                                                  DBusMessage *message);

Could you align the arguments? Thanks!

::: daemon/dbus/gkd-secret-objects.c
@@ +59,3 @@
+gpointer
+master_secret_for_collection (GkdSecretService *service, GckObject *collection,
+                              gsize *n_secret, GError **error)

As noted earlier, in new code function definitions please wrap one argument per line. Thanks!

::: daemon/dbus/gkd-secret-objects.h
@@ +44,3 @@
+gpointer
+master_secret_for_collection (GkdSecretService *service, GckObject *collection,
+                              gsize *n_secret, GError **error);

When we make a function public, we generally name it with a prefix to help developers easily know what functionality and file it belongs to.

Could you prefix this appropriately similar to the other functions in this file.

Could you also wrap it and place it along side the other functions declarations in this file?

Thank you.
Comment 7 Or Goshen 2011-09-25 12:17:57 UTC
Created attachment 197424 [details] [review]
krb5 patch v3

krb5 patch v3
Comment 8 Or Goshen 2011-09-25 13:32:01 UTC
Created attachment 197426 [details] [review]
krb5 patch V4

Fixed a typo in gdk-secret.kerberos.h
Comment 9 Stef Walter 2011-09-26 06:23:04 UTC
Looking good. I'd like to be able to see how to test this before merging it. See my request above? Is that realistic?
Comment 10 Or Goshen 2011-09-27 09:29:48 UTC
Created attachment 197552 [details] [review]
krb5 patch V5

Fixes conflicts with latest updates from the master branch
Comment 11 Or Goshen 2011-10-03 08:10:51 UTC
Docummentation
--------------

Purpose:
Kerberos authentication has become more commonly used in the recent years. In MS world NTLM authentication became obsolete and Kerberos is replacing it. Since Gnome-Keyring is a repository for secrets it's just natural that it will be used during the process of getting a TGT (Tickets Granting Ticket - more on this in the Kerberos standard or implementations).

Implementation:
We had 3 guidelines: 1. handing over the clear text passord to anyone is undesireable. 2. Gnome-Keyring purpose is only to store secrets, anything else such as communicating over the network with the KDC (key distribution center) isnt in its job description. 3. Gnome-Keyring should be as simple as possible and do minimal math on its side.
therefore an interface with a single method was designed.
The interface is "org.gnome.keyring.Credential.Kerberos" and the method is called "GetHashedKey". The method accepts 2 input parameters: encryption type & salt and returns the hashed key (as returned by the krb5 function krb5_c_string_to_key).
The RPC mechanism used is DBus.

How to test:
1. Setup a keyring with the password "flexqa".
2. Execute the following command (as a regular user):
dbus-send --print-reply --session --dest=org.freedesktop.secrets /org/freedesktop/secrets/collection/login org.gnome.keyring.Credential.Kerberos.GetHashedKey int32:18 array:byte:12,13,14
3. Expected result:
method return sender=:1.22 -> dest=:1.138 reply_serial=2
   array of bytes [
      06 70 da 74 d4 f1 7a 6b 9d 2e dd 97 69 25 0e d2 18 9e 64 51 65 a9 b6 03
      c7 15 8e de 47 84 c6 5b
   ]
Comment 12 Stef Walter 2011-10-03 08:13:54 UTC
The docs look good. Could you add this to the file HOWTO_KERB5 in the daemon/dbus/tests directory.
Comment 13 Or Goshen 2011-10-03 08:41:49 UTC
Created attachment 198063 [details] [review]
Related documentation
Comment 14 Stef Walter 2011-10-03 18:55:32 UTC
heimdal doesn't seem to have git grep krb5_c_string_to_key(). Am I missing something, this is how I checked:

$ git clone git@github.com:heimdal/heimdal.git
$ cd heimdal
$ git grep krb5_c_string_to_key

Did you check that this builds with heimdal? If it can't work with heimdal, then should we remove support for heimdal in the configure script?
Comment 15 Stef Walter 2011-10-03 19:00:09 UTC
Created attachment 198140 [details] [review]
I've updated the patch with a bit of cleanup. In particular:

* Bourne shell test does not support ==
 * Functions in headers should generally be namespaced
 * Cleanup the how to commands so they actually work when copy/pasted

Other than that, I tested this with mitkrb5 and it returns the hash
as expected. Looking great.

Out of interest, where's the code that calls this dbus method. In
krb5-auth-dialog? Have you developed that yet? Just interested in
taking a look.
Comment 16 Stef Walter 2011-10-21 18:42:23 UTC
Should I strip out the heimdal support (until someone steps up to test that) and merge the patch without it?
Comment 17 Stef Walter 2012-03-15 09:48:38 UTC
No further response. Closing this bug for now, but please do reopen if you're interested in completing the work.

FWIW additional further work is being done on intergrating kerberos acquisition and renewal in GNOME: http://live.gnome.org/Design/Proposals/UserIdentities
Comment 18 Stef Walter 2012-09-19 08:26:18 UTC
Created attachment 224708 [details] [review]
Updated the patch to work with git master.

Although I'm not sure if this still makes sense given all the kerberos work
going in on in gnome-online-accounts.

Add support for Kerberos V5 key hashing

 * See daemon/dbus/tests/HOWTO_KRB5 for details on how to
   test this feature