New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add ability to store extended principal attributes in LDAP #141
Conversation
if (extensions != NULL) { | ||
size_t l; | ||
|
||
ent->entry.extensions = malloc(sizeof(*(ent->entry.extensions))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check return value, use ret = krb5_enomem(context);\ngoto out;
fix review comments and I'm happy to merge, thanks for fixing this. preferably the extension should be stored as native ldap options, but this will do for now |
Should be fixed now. I don't see how we can reasonably break up certain attributes, such as the PKI alias fields, due to their SEQUENCE of SEQUENCE construction. LDAP only "understands" either a single value per attribute or an unordered list of attributes; trying to add unordered lists of PKI alias fields would require adding a completely new (non-derivative) data type to LDAP, which is unfortunately beyond my capabilities at this time. |
for (i = 0; i < ent->entry.extensions->len; i++) { | ||
int asn1_decode_ret = decode_HDB_extension((unsigned char *) extensions[i]->bv_val, | ||
(size_t) extensions[i]->bv_len, &ent->entry.extensions->val[i], &l); | ||
if (asn1_decode_ret < 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the correct check here is actually != 0 or just if (asn1_decode_ret) {
I think you should just use ret directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going on the documentation here:
http://bellard.org/ffasn1/ffasn1c.html#TOC10
Specifically, "Return the number of consumed bytes or < 0 if error.". Does Heimdal's ASN1 parser behave differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A careful code review was undertaken, and it was determined that the best way to store the extended attributes was in a native ASN1 encoded field. LDAP does not understand the SEQUENCE of SEQUENCE structures used extensively throughout the extended attributes structure, and there was already a precedent set for storing the krb5Key data in a native ASN1 encoded field.
OK, updated. |
Add ability to store extended principal attributes in LDAP
When attempting to deploy PKI infrastructure on our Kerbeos servers with LDAP backends it was discovered that the Heimdal Kerbeos extended attribute structure (including PKI mapping fields) was not supported with the LDAP backend. This patch fixes this critical issue.
Original commit text:
A careful code review was undertaken, and it was determined
that the best way to store the extended attributes was in a
native ASN1 encoded field. LDAP does not understand the
SEQUENCE of SEQUENCE structures used extensively throughout
the extended attributes structure, and there was already a
precedent set for storing the krb5Key data in a native ASN1
encoded field.