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] [Updated] (DIRKRB-692) 1)sgtTicket clientPrincipal is not initialized + 2)KrbClient fails to store SGT ticket in cache file
Date Fri, 09 Feb 2018 07:15:01 GMT

     [ https://issues.apache.org/jira/browse/DIRKRB-692?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Fabiano Tarlao updated DIRKRB-692:
----------------------------------
    Description: 
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 (the real complete fix is on GitHub,in a set of subsequent pull requests :) ):

 
{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:

 
{code:java}
public void storeTicket(SgtTicket sgtTicket, File ccacheFile) throws KrbException {
        LOG.info("Storing the sgt to the credential cache file.");
        boolean isFreshNew = !ccacheFile.exists() || (ccacheFile.length() == 0);
        if (isFreshNew) {
            createCacheFile(ccacheFile);
        }
        if (ccacheFile.exists() && ccacheFile.canWrite()) {
            
            try {
                CredentialCache cCache;
                if (!isFreshNew) {
                    cCache = new CredentialCache(); 
                    cCache.load(ccacheFile);
                    cCache.addCredential(new Credential(sgtTicket, sgtTicket.getClientPrincipal()));
                } else {
                    //Remind: contructor sets the cCache client principal
from the sgtTicket one
                    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());
        }
    }
{code}
 

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

  was:
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


> 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
>             Fix For: 1.0.2, 1.1.1
>
>         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 (the real complete fix is on GitHub,in a set of subsequent pull requests :) ):
>  
> {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:
>  
> {code:java}
> public void storeTicket(SgtTicket sgtTicket, File ccacheFile) throws KrbException {
>         LOG.info("Storing the sgt to the credential cache file.");
>         boolean isFreshNew = !ccacheFile.exists() || (ccacheFile.length() == 0);
>         if (isFreshNew) {
>             createCacheFile(ccacheFile);
>         }
>         if (ccacheFile.exists() && ccacheFile.canWrite()) {
>             
>             try {
>                 CredentialCache cCache;
>                 if (!isFreshNew) {
>                     cCache = new CredentialCache(); 
>                     cCache.load(ccacheFile);
>                     cCache.addCredential(new Credential(sgtTicket,
sgtTicket.getClientPrincipal()));
>                 } else {
>                     //Remind: contructor sets the cCache client principal
from the sgtTicket one
>                     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());
>         }
>     }
> {code}
>  
> 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