Bug 811375
| Summary: | Use keytab to select etypes for krb5_get_init_creds_keytab() | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Stef Walter <stefw> | ||||
| Component: | sssd | Assignee: | Stephen Gallagher <sgallagh> | ||||
| Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> | ||||
| Severity: | high | Docs Contact: | |||||
| Priority: | unspecified | ||||||
| Version: | rawhide | CC: | jhrozek, ondrejv, sbose, sgallagh, ssorce | ||||
| Target Milestone: | --- | ||||||
| Target Release: | --- | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Whiteboard: | |||||||
| Fixed In Version: | Doc Type: | Bug Fix | |||||
| Doc Text: | Story Points: | --- | |||||
| Clone Of: | |||||||
| : | 956766 (view as bug list) | Environment: | |||||
| Last Closed: | 2012-06-15 00:34:35 UTC | Type: | Bug | ||||
| Regression: | --- | Mount Type: | --- | ||||
| Documentation: | --- | CRM: | |||||
| Verified Versions: | Category: | --- | |||||
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||
| Cloudforms Team: | --- | Target Upstream Version: | |||||
| Embargoed: | |||||||
| Bug Depends On: | |||||||
| Bug Blocks: | 956766 | ||||||
| Attachments: |
|
||||||
|
Description
Stef Walter
2012-04-10 20:26:35 UTC
Created attachment 576590 [details]
Patch which fixes the problem
Hi Stef, first, thank you very much for the patch! I haven't tested it, but from casual read it seems quite good. Would you be willing to tailor it so that it conforms to our coding style? The full document can be seen at http://www.freeipa.org/page/Coding_Style There are two things that would be nice to have changed: * the temporary memory context allocated on NULL is usually called tmp_ctx * we tend to use curly brackets even if there is only one statement after a loop or an if (not always, but usually, exceptions being shorthands such as if(!ptr) return ENOMEM) Thanks for the patch, Stef. I have a few additional comments: * As a general style preference, please don't increment at the same time as assignment. Prefer: etypes[count] = entry.key.enctype; count++; over etypes[count++] = entry.key.enctype; While they are functionally identical, the former is easier to grok at a quick glance. * Please keep lines to 79 characters * Please open an RFE against Kerberos to export the internal function to identify which enctypes are stronger. * You're returning type krb5_error_code, but you also return ENOMEM in one place. Please replace this with an appropriate krb5_error_code value. * This function does not handle memory well if it fails at any point. It would be better to allocate on a tmp_ctx and then talloc_steal() onto mem_ctx only just before returning. Right now, if krb5_kt_next_entry() ever returns anything but KRB5_KT_END or if talloc_realloc() fails (after the first one), you'll be effectively leaking any memory that was allocated by talloc_realloc(). (In reality, it will still be attached to mem_ctx but unreachable and unfreeable until mem_ctx is freed). * You need to check that the final talloc_realloc() succeeds. * I feel like it would be a cleaner interface if sss_krb5_read_etypes_for_keytab() returns the number of entries in the etype_list, rather than using talloc_array_length(), which will include the NULL entry. * On a related note, you have a comment stating that you will NULL-terminate the list, but never do. Either the comment or logic is wrong. Finally, we generally prefer that patches are submitted to upstream via https://fedorahosted.org/mailman/listinfo/sssd-devel so they are more visible. Upstream ticket: https://fedorahosted.org/sssd/ticket/1297 Stef, how did you create the keytab in the first place ? Although I am find making SSSD more robust, keytabs are supposed to contin exclusively kys available also on the KDC. so having an ecntype not available there is a bug in the keytab creation more than a bug in applications using it. The keytab was created with samba3's: net ads keytab create The keytab contains: Keytab name: FILE:/data/build/etc/krb5.keytab KVNO Principal ---- -------------------------------------------------------------------------- 2 host/stef-desktop.thewalter.lan.LAN (des-cbc-crc) 2 host/stef-desktop.thewalter.lan.LAN (des-cbc-md5) 2 host/stef-desktop.thewalter.lan.LAN (arcfour-hmac) 2 host/stef-desktop.LAN (des-cbc-crc) 2 host/stef-desktop.LAN (des-cbc-md5) 2 host/stef-desktop.LAN (arcfour-hmac) 2 STEF-DESKTOP$@AD.THEWALTER.LAN (des-cbc-crc) 2 STEF-DESKTOP$@AD.THEWALTER.LAN (des-cbc-md5) 2 STEF-DESKTOP$@AD.THEWALTER.LAN (arcfour-hmac) 3 host/stef-desktop.thewalter.lan.LAN (des-cbc-crc) 3 host/stef-desktop.thewalter.lan.LAN (des-cbc-md5) 3 host/stef-desktop.thewalter.lan.LAN (arcfour-hmac) 3 host/stef-desktop.LAN (des-cbc-crc) 3 host/stef-desktop.LAN (des-cbc-md5) 3 host/stef-desktop.LAN (arcfour-hmac) 3 STEF-DESKTOP$@AD.THEWALTER.LAN (des-cbc-crc) 3 STEF-DESKTOP$@AD.THEWALTER.LAN (des-cbc-md5) 3 STEF-DESKTOP$@AD.THEWALTER.LAN (arcfour-hmac) Even if the keytab is not 'technically' correct, I'd like to see this become less brittle and more maintenance free. Barely anyone understands these Kerberos issues and terminology, especially the AD admins and users we're targetting. So issues like this shouldn't come up in the wild. Btw Stephen, thanks for the review. Doing some work upstream on this. Will complete that before continuing on this bug. Verified this bug still exists in F17 Fix committed upstream. sssd-1.8.4-12.fc17 has been submitted as an update for Fedora 17. https://admin.fedoraproject.org/updates/sssd-1.8.4-12.fc17 sssd-1.8.4-12.fc16 has been submitted as an update for Fedora 16. https://admin.fedoraproject.org/updates/sssd-1.8.4-12.fc16 Package sssd-1.8.4-12.fc16: * should fix your issue, * was pushed to the Fedora 16 testing repository, * should be available at your local mirror within two days. Update it with: # su -c 'yum update --enablerepo=updates-testing sssd-1.8.4-12.fc16' as soon as you are able to. Please go to the following url: https://admin.fedoraproject.org/updates/FEDORA-2012-8700/sssd-1.8.4-12.fc16 then log in and leave karma (feedback). sssd-1.8.4-12.fc16 has been pushed to the Fedora 16 stable repository. If problems still persist, please make note of it in this bug report. sssd-1.8.4-12.fc17 has been pushed to the Fedora 17 stable repository. If problems still persist, please make note of it in this bug report. |