directory-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Fabiano Tarlao (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (DIRKRB-692) 1)sgtTicket clientPrincipal is not initialized + 2)KrbClient fails to store SGT ticket in cache file
Date Wed, 07 Feb 2018 15:54:00 GMT

    [ https://issues.apache.org/jira/browse/DIRKRB-692?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16355619#comment-16355619
] 

Fabiano Tarlao edited comment on DIRKRB-692 at 2/7/18 3:53 PM:
---------------------------------------------------------------

I cannot provide a final test for your project, I have not understood how your integration
tests work. I think you spawn a local kdc instance with predefined domain users.

I have provided a normal test KrbClientTest.java that assumes a kdc is already running somewhere
and KrbClientTest.java class contains constants that should be initialized properly...

It also loads the kdc configuration from an available kerb5.conf. You can also change that
part to a getResource... + new File(...getURI)... It depends on your best practises about.
Hope useful

Regards


was (Author: ftarlao):
I cannot provide a final test for your project, I have not understood how your integration
tests work. I think you spawn a local kdc instance with predefined domain users.

I have provided a normal test KrbClientTest.java that assumes a kdc is already running somewhere
and that class has constants that should be initialized properly... [|https://issues.apache.org/jira/secure/DeleteAttachment!default.jspa?id=13136579&deleteAttachmentId=12909628&from=issue]it
also loads the kdc configuration from an available kerb5.conf. You can also change that part
to a getResource... new File(...getURI)... I have currently no-idea of your best practises
about. Hope useful

Regards

> 1)sgtTicket clientPrincipal is not initialized + 2)KrbClient fails to store SGT ticket
in cache file
> ----------------------------------------------------------------------------------------------------
>
>                 Key: DIRKRB-692
>                 URL: https://issues.apache.org/jira/browse/DIRKRB-692
>             Project: Directory Kerberos
>          Issue Type: Bug
>    Affects Versions: 1.1.0
>         Environment: Linux Mint 17.1 + Netbeans 8.1 with a Maven Project + kerb-core
1.1.0 in Windows AD (KDC is Windows 2016 Server)
>            Reporter: Fabiano Tarlao
>            Priority: Major
>              Labels: easyfix
>         Attachments: KrbClientTest.java, bug1.diff, bug2.diff
>
>
> Two bugs that interact each other
> h1. 1)
> *SgtTicket*, returned by *KrbClient.requestSgt(..)*, has a _null_ *clientPrincipal* field
(unassigned). Perhaps this is not mandatory but your code assumes this property is populated
(see later). I highly suggest to populate this field.
> I have wrote a workaround that overrides the *requestSgt* method and works for the *USE_TGT*
case:
>  
> {code:java}
> @Override
>     public SgtTicket requestSgt(KOptions requestOptions) throws KrbException {
>         SgtTicket requestSgt = super.requestSgt(requestOptions); 
>         TgtTicket tgt = (TgtTicket) requestOptions.getOptionValue(KrbOption.USE_TGT);
>         if(tgt != null){
>             requestSgt.setClientPrincipal(tgt.getClientPrincipal());
>         }
>         return requestSgt;
>     }{code}
>  
> h1. 2)
> Creating a new credential cache file when storing only one SGT (service ticket) fails.
(i.e., trying to create a new cache file containing only one SGT and no TGT)
> Invoking *KrbClient.storeTicket(sgtTicket, File)* fails for this reason (here is the
original code in *KrbClientBase* class, my comments in RED ):
> {{public void storeTicket(SgtTicket sgtTicket, File ccacheFile) throws KrbException {}}
>  {{        LOG.info("Storing the sgt to the credential cache file.");}}
>  {{        if (!ccacheFile.exists()) {}}
>  {{            createCacheFile(ccacheFile);{color:#ff0000} //Correctly creates
a new file but...{color}}}
>  {{        if (ccacheFile.exists() && ccacheFile.canWrite()) {}}
>  {{            CredentialCache cCache = new CredentialCache();}}
>  {{            try {}}
>  {{                cCache.load(ccacheFile);{color:#ff0000} //..this line
EXPLODES cause it tries to initialize from an empty file, the unexistent file case is not
managed correctly{color}}}
>  {{                cCache.addCredential(new Credential(sgtTicket, sgtTicket.getClientPrincipal()));}}
>  {{                cCache.setPrimaryPrincipal(sgtTicket.getClientPrincipal());}}
>  {{                cCache.store(ccacheFile);}}
>  {{            } catch (IOException e) {}}
>  {{                throw new KrbException("Failed to store sgt", e);}}
> {{        } else {}}
>  {{            throw new IllegalArgumentException("Invalid ccache file, "}}
>  {{                    + "not exist or writable: " + ccacheFile.getAbsolutePath());}}
>  {{}}}
> Here follows my proposal/fix, this code correctly manages the MIT ccache file creation
for one SGT, please note that this fix assumes that bug 1 is already fixed:
> {{public static void storeTicket(SgtTicket sgtTicket, File ccacheFile) throws KrbException
{}}
>  {{        LOG.info("Storing the sgt to the credential cache file.");}}
>  {{        boolean isFreshNew = !ccacheFile.exists();}}
>  {{        if (isFreshNew) {}}
>  {{            createCacheFile(ccacheFile);}}
>  {{        if (ccacheFile.exists() && ccacheFile.canWrite()) {}}
>  {{            }}
>  {{            try {}}
>  {{                CredentialCache cCache;}}
>  {{                if(!isFreshNew){}}
>  {{                    cCache = new CredentialCache(sgtTicket); {color:#ff0000}//This
constructor sets also the cCache principal from sgtTicket principal{color}}}
>  {{                    cCache.load(ccacheFile);}}
>  {{                    cCache.addCredential(new Credential(sgtTicket,
sgtTicket.getClientPrincipal()));}}
>  {{                    cCache.setPrimaryPrincipal(sgtTicket.getClientPrincipal());}}
>  {{                } else {}}
>  {{                    cCache = new CredentialCache(sgtTicket);}}
>  {{                cCache.store(ccacheFile);}}
>  {{            } catch (IOException e) {}}
>  {{                throw new KrbException("Failed to store sgt", e);}}
>  {{        } else {}}
>  {{            throw new IllegalArgumentException("Invalid ccache file, "}}
>  {{                    + "not exist or writable: " + ccacheFile.getAbsolutePath());}}
> Please note that *YOUR CredentialCache contructor assumes the clientPrincipal is populated*
;)
> Hope useful,
> regards
> Fabiano
> P.s: Attached two paches for the project, bug2.diff for bug 2 is..*I think.. production
ready*.
> bug1.diff for bug 1 is untested.. I ported the modification of my overriding method to
the *AbstractInternalKrbClient.requestSgt()* method. But I have not performed run/test of
modules, your project is big and needs time to manage well. *BUT* is this the right solution?
or SHOULD the *clientPrincipal* be already populated from the doRequestSgt method? I suggest
to look for a solution at a deeper level starting from *DefaultInternalKrbClient*. Consider
bug1.diff a fair workaround.
> P.s.2:Added a pull request on GitHub
> Regards



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message