hdb: Use a CHOICE instead of ASN1_BAD_ID

Checking the error code of decoding a blob as an hdb_entry or
hdb_entry_alias to determine which of those the blob is depends on a
detail of the Heimdal ASN.1 compiler and library that shouldn't be
depended on.  Using a CHOICE adds no octets to the encoding:

    HDB-EntryOrAlias ::= CHOICE {
            entry       hdb_entry,
            alias       hdb_entry_alias
    }

since we're adding no additional tags and the two arms of the CHOICE
already differ in tag (hdb_entry's tag is a [UNIVERSAL Sequence] tag,
while hdb_entry_alias's is an [APPLICATION 0] tag).

lib/hdb/Makefile.am

@@ -35,6 +35,7 @@ gen_files_hdb = \
 	asn1_HDB_extension.x \
 	asn1_HDB_extensions.x \
 	asn1_HDB_EncTypeList.x \
+	asn1_HDB_EntryOrAlias.x \
 	asn1_KeyRotation.x \
 	asn1_KeyRotationFlags.x \
 	asn1_hdb_entry.x \

lib/hdb/common.c

@@ -143,14 +143,22 @@ add_default_salts(krb5_context context, HDB *db, hdb_entry *entry)
     return ret;
 }
 
-krb5_error_code
+static krb5_error_code
-_hdb_fetch_kvno(krb5_context context, HDB *db, krb5_const_principal principal,
+fetch_entry_or_alias(krb5_context context,
-		unsigned flags, krb5_kvno kvno, hdb_entry_ex *entry)
+                     HDB *db,
+                     krb5_const_principal principal,
+                     unsigned flags,
+                     hdb_entry_ex *entry)
 {
+    HDB_EntryOrAlias eoa;
     krb5_principal enterprise_principal = NULL;
     krb5_data key, value;
     krb5_error_code ret;
 
+    value.length = 0;
+    value.data = 0;
+    key = value;
+
     if (principal->name.name_type == KRB5_NT_ENTERPRISE_PRINCIPAL) {
 	if (principal->name.name_string.len != 1) {
 	    ret = KRB5_PARSE_MALFORMED;
@@ -166,41 +174,58 @@ _hdb_fetch_kvno(krb5_context context, HDB *db, krb5_const_principal principal,
 	principal = enterprise_principal;
     }
 
-    hdb_principal2key(context, principal, &key);
+    ret = hdb_principal2key(context, principal, &key);
-    if (enterprise_principal)
+    if (ret == 0)
-	krb5_free_principal(context, enterprise_principal);
         ret = db->hdb__get(context, db, key, &value);
+    if (ret == 0)
+        ret = decode_HDB_EntryOrAlias(value.data, value.length, &eoa, NULL);
+    if (ret == 0 && eoa.element == choice_HDB_EntryOrAlias_entry) {
+        entry->entry = eoa.u.entry;
+    } else if (ret == 0 && eoa.element == choice_HDB_EntryOrAlias_alias) {
         krb5_data_free(&key);
-    if(ret)
+	ret = hdb_principal2key(context, eoa.u.alias.principal, &key);
-	return ret;
+        if (ret == 0)
+            ret = db->hdb__get(context, db, key, &value);
+        if (ret == 0)
+            /* No alias chaining */
             ret = hdb_value2entry(context, &value, &entry->entry);
-    /* HDB_F_GET_ANY indicates request originated from KDC (not kadmin) */
+    } else if (ret == 0)
-    if (ret == ASN1_BAD_ID && (flags & (HDB_F_CANON|HDB_F_GET_ANY)) == 0) {
+        ret = ENOTSUP;
-	krb5_data_free(&value);
+    if (ret == 0 && enterprise_principal) {
-	return HDB_ERR_NOENTRY;
+	/*
-    } else if (ret == ASN1_BAD_ID) {
+	 * Whilst Windows does not canonicalize enterprise principal names if
-	hdb_entry_alias alias;
+	 * the canonicalize flag is unset, the original specification in
-
+	 * draft-ietf-krb-wg-kerberos-referrals-03.txt says we should.
-	ret = hdb_value2entry_alias(context, &value, &alias);
+	 */
-	if (ret) {
+	entry->entry.flags.force_canonicalize = 1;
-	    krb5_data_free(&value);
-	    return ret;
     }
-	hdb_principal2key(context, alias.principal, &key);
-	krb5_data_free(&value);
-	free_hdb_entry_alias(&alias);
 
-	ret = db->hdb__get(context, db, key, &value);
+    /* HDB_F_GET_ANY indicates request originated from KDC (not kadmin) */
+    if (ret == 0 && eoa.element == choice_HDB_EntryOrAlias_alias &&
+        (flags & (HDB_F_CANON|HDB_F_GET_ANY)) == 0) {
+
+        /* `principal' was alias but canon not req'd */
+        free_hdb_entry(&entry->entry);
+        ret = HDB_ERR_NOENTRY;
+    }
+
+    krb5_free_principal(context, enterprise_principal);
+    krb5_data_free(&value);
     krb5_data_free(&key);
+    principal = enterprise_principal = NULL;
+    return ret;
+}
+
+krb5_error_code
+_hdb_fetch_kvno(krb5_context context, HDB *db, krb5_const_principal principal,
+		unsigned flags, krb5_kvno kvno, hdb_entry_ex *entry)
+{
+    krb5_error_code ret;
+
+    ret = fetch_entry_or_alias(context, db, principal, flags, entry);
     if (ret)
         return ret;
-	ret = hdb_value2entry(context, &value, &entry->entry);
+
-	if (ret) {
-	    krb5_data_free(&value);
-	    return ret;
-	}
-    }
-    krb5_data_free(&value);
     if ((flags & HDB_F_DECRYPT) && (flags & HDB_F_ALL_KVNOS)) {
 	/* Decrypt the current keys */
 	ret = hdb_unseal_keys(context, db, &entry->entry);
@@ -249,14 +274,6 @@ _hdb_fetch_kvno(krb5_context context, HDB *db, krb5_const_principal principal,
 	    return ret;
 	}
     }
-    if (enterprise_principal) {
-	/*
-	 * Whilst Windows does not canonicalize enterprise principal names if
-	 * the canonicalize flag is unset, the original specification in
-	 * draft-ietf-krb-wg-kerberos-referrals-03.txt says we should.
-	 */
-	entry->entry.flags.force_canonicalize = 1;
-    }
 
     return 0;
 }
@@ -336,48 +353,47 @@ hdb_add_aliases(krb5_context context, HDB *db,
     return 0;
 }
 
+/* Check if new aliases are already used for other entries */
 static krb5_error_code
 hdb_check_aliases(krb5_context context, HDB *db, hdb_entry_ex *entry)
 {
-    const HDB_Ext_Aliases *aliases;
+    const HDB_Ext_Aliases *aliases = NULL;
-    int code;
+    HDB_EntryOrAlias eoa;
-    size_t i;
-
-    /* check if new aliases already is used */
-
-    code = hdb_entry_get_aliases(&entry->entry, &aliases);
-    if (code)
-	return code;
-
-    for (i = 0; aliases && i < aliases->aliases.len; i++) {
-	hdb_entry_alias alias;
     krb5_data akey, value;
+    size_t i;
+    int ret;
 
-	code = hdb_principal2key(context, &aliases->aliases.val[i], &akey);
+    memset(&eoa, 0, sizeof(eoa));
-        if (code == 0) {
+    krb5_data_zero(&value);
-            code = db->hdb__get(context, db, akey, &value);
+    akey = value;
+
+    ret = hdb_entry_get_aliases(&entry->entry, &aliases);
+    for (i = 0; ret == 0 && aliases && i < aliases->aliases.len; i++) {
+	ret = hdb_principal2key(context, &aliases->aliases.val[i], &akey);
+        if (ret == 0)
+            ret = db->hdb__get(context, db, akey, &value);
+        if (ret == 0)
+            ret = decode_HDB_EntryOrAlias(value.data, value.length, &eoa, NULL);
+        if (ret == 0 && eoa.element != choice_HDB_EntryOrAlias_entry &&
+            eoa.element != choice_HDB_EntryOrAlias_alias)
+            ret = ENOTSUP;
+        if (ret == 0 && eoa.element == choice_HDB_EntryOrAlias_entry)
+            /* New alias names an existing non-alias entry in the HDB */
+            ret = HDB_ERR_EXISTS;
+        if (ret == 0 && eoa.element == choice_HDB_EntryOrAlias_alias &&
+            !krb5_principal_compare(context, eoa.u.alias.principal,
+                                    entry->entry.principal))
+            /* New alias names an existing alias of a different entry */
+            ret = HDB_ERR_EXISTS;
+        if (ret == HDB_ERR_NOENTRY) /* from db->hdb__get */
+            /* New alias is a name that doesn't exist in the HDB */
+            ret = 0;
+
+        free_HDB_EntryOrAlias(&eoa);
+	krb5_data_free(&value);
         krb5_data_free(&akey);
     }
-	if (code == HDB_ERR_NOENTRY)
+    return ret;
-	    continue;
-	else if (code)
-	    return code;
-
-	code = hdb_value2entry_alias(context, &value, &alias);
-	krb5_data_free(&value);
-
-	if (code == ASN1_BAD_ID)
-	    return HDB_ERR_EXISTS;
-	else if (code)
-	    return code;
-
-	code = krb5_principal_compare(context, alias.principal,
-				      entry->entry.principal);
-	free_hdb_entry_alias(&alias);
-	if (code == 0)
-	    return HDB_ERR_EXISTS;
-    }
-    return 0;
 }
 
 /*

lib/hdb/hdb.asn1

@@ -240,4 +240,9 @@ hdb_entry_alias ::= [APPLICATION 0] SEQUENCE {
 	principal[0]	Principal  OPTIONAL
 }
 
+HDB-EntryOrAlias ::= CHOICE {
+        entry       hdb_entry,
+        alias       hdb_entry_alias
+}
+
 END

lib/hdb/libhdb-exports.def

@@ -107,12 +107,18 @@ EXPORTS
 	asn1_HDBFlags_units
 	copy_Event
 	copy_HDB_EncTypeList
+	copy_hdb_entry
+	copy_hdb_entry_alias
+	copy_HDB_EntryOrAlias
 	copy_HDB_extensions
 	copy_HDB_Ext_KeyRotation
 	copy_Key
 	copy_Keys
 	copy_Salt
 	decode_HDB_EncTypeList
+	decode_hdb_entry
+	decode_hdb_entry_alias
+	decode_HDB_EntryOrAlias
 	decode_HDB_Ext_Aliases
 	decode_HDB_extension
 	decode_HDB_Ext_KeyRotation
@@ -120,6 +126,9 @@ EXPORTS
 	decode_Key
 	decode_Keys
 	encode_HDB_EncTypeList
+	encode_hdb_entry
+	encode_hdb_entry_alias
+	encode_HDB_EntryOrAlias
 	encode_HDB_Ext_Aliases
 	encode_HDB_extension
 	encode_HDB_Ext_KeyRotation
@@ -129,6 +138,8 @@ EXPORTS
 	free_Event
 	free_HDB_EncTypeList
 	free_hdb_entry
+	free_hdb_entry_alias
+	free_HDB_EntryOrAlias
 	free_HDB_Ext_Aliases
 	free_HDB_extension
 	free_HDB_extensions
@@ -144,6 +155,9 @@ EXPORTS
 	int2KeyRotationFlags
 	KeyRotationFlags2int
 	length_HDB_EncTypeList
+	length_hdb_entry
+	length_hdb_entry_alias
+	length_HDB_EntryOrAlias
 	length_HDB_Ext_Aliases
 	length_HDB_extension
 	length_HDB_Ext_KeyRotation

lib/hdb/test_dbinfo.c

@@ -44,6 +44,65 @@ struct getargs args[] = {
 
 static int num_args = sizeof(args) / sizeof(args[0]);
 
+/*
+ * Prove that HDB_EntryOrAlias being a CHOICE of hdb_entry or hdb_entry_alias
+ * adds nothing to the encoding of those types.
+ */
+static
+void
+check_HDB_EntryOrAlias(krb5_context context)
+{
+    HDB_EntryOrAlias eoa;
+    hdb_entry entry;
+    hdb_entry_alias alias;
+    krb5_data v;
+    size_t len;
+    int ret;
+
+    memset(&entry, 0, sizeof(entry));
+    memset(&alias, 0, sizeof(alias));
+    memset(&eoa, 0, sizeof(eoa));
+    krb5_data_zero(&v);
+
+    ret = krb5_make_principal(context, &alias.principal, "KTH.SE", "foo",
+                              NULL);
+    if (ret)
+        krb5_err(context, 1, ret, "krb5_make_principal");
+    ASN1_MALLOC_ENCODE(hdb_entry_alias, v.data, v.length, &alias, &len, ret);
+    if (ret)
+        krb5_err(context, 1, ret, "encode_HDB_EntryOrAlias");
+    if (v.length != len)
+        abort();
+    ret = decode_HDB_EntryOrAlias(v.data, v.length, &eoa, &len);
+    if (ret)
+        krb5_err(context, 1, ret, "decode_HDB_EntryOrAlias");
+    if (v.length != len)
+        abort();
+    free_HDB_EntryOrAlias(&eoa);
+    free_hdb_entry_alias(&alias);
+    krb5_data_free(&v);
+
+    ret = krb5_make_principal(context, &entry.principal, "KTH.SE", "foo",
+                              NULL);
+    if (ret)
+        krb5_err(context, 1, ret, "krb5_make_principal");
+    entry.kvno = 5;
+    entry.flags.initial = 1;
+    ASN1_MALLOC_ENCODE(hdb_entry, v.data, v.length, &entry, &len, ret);
+    if (ret)
+        krb5_err(context, 1, ret, "encode_HDB_EntryOrAlias");
+    if (v.length != len)
+        abort();
+    ret = decode_HDB_EntryOrAlias(v.data, v.length, &eoa, &len);
+    if (ret)
+        krb5_err(context, 1, ret, "decode_HDB_EntryOrAlias");
+    if (v.length != len)
+        abort();
+    free_HDB_EntryOrAlias(&eoa);
+    free_hdb_entry(&entry);
+    krb5_data_free(&v);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -68,6 +127,8 @@ main(int argc, char **argv)
     if (ret)
 	errx (1, "krb5_init_context failed: %d", ret);
 
+    check_HDB_EntryOrAlias(context);
+
     ret = hdb_get_dbinfo(context, &info);
     if (ret)
 	krb5_err(context, 1, ret, "hdb_get_dbinfo");

lib/hdb/version-script.map

@@ -111,12 +111,18 @@ HEIMDAL_HDB_1.0 {
 		asn1_HDBFlags_units;
 		copy_Event;
 		copy_HDB_EncTypeList;
+		copy_hdb_entry;
+		copy_hdb_entry_alias;
+		copy_HDB_EntryOrAlias;
 		copy_HDB_extensions;
 		copy_HDB_Ext_KeyRotation;
 		copy_Key;
 		copy_Keys;
 		copy_Salt;
 		decode_HDB_EncTypeList;
+		decode_hdb_entry;
+		decode_hdb_entry_alias;
+		decode_HDB_EntryOrAlias;
 		decode_HDB_Ext_Aliases;
 		decode_HDB_extension;
 		decode_HDB_Ext_KeyRotation;
@@ -124,6 +130,9 @@ HEIMDAL_HDB_1.0 {
 		decode_Key;
 		decode_Keys;
 		encode_HDB_EncTypeList;
+		encode_hdb_entry;
+		encode_hdb_entry_alias;
+		encode_HDB_EntryOrAlias;
 		encode_HDB_Ext_Aliases;
 		encode_HDB_extension;
 		encode_HDB_Ext_KeyRotation;
@@ -134,6 +143,8 @@ HEIMDAL_HDB_1.0 {
 		free_Event;
 		free_HDB_EncTypeList;
 		free_hdb_entry;
+		free_hdb_entry_alias;
+		free_HDB_EntryOrAlias;
 		free_HDB_Ext_Aliases;
 		free_HDB_extension;
 		free_HDB_extensions;
@@ -149,6 +160,9 @@ HEIMDAL_HDB_1.0 {
 		int2KeyRotationFlags;
 		KeyRotationFlags2int;
 		length_HDB_EncTypeList;
+		length_hdb_entry;
+		length_hdb_entry_alias;
+		length_HDB_EntryOrAlias;
 		length_HDB_Ext_Aliases;
 		length_HDB_extension;
 		length_HDB_Ext_KeyRotation;