Skip to content
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

Removed unsafe package and native kerberos ticket check #2363

Merged
merged 18 commits into from
Dec 20, 2021

Conversation

davecramer
Copy link
Member

Still not sure what to do with the configuration. Currently we think the name is pgjdbc for the service. This should probably be consistent

Copy link

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me what the intent of re-using the JAAS_APPLICATION_NAME's config is needed. It didn't use it before.


public class KerberosTicket {

private static final String CONFIG_ITEM_NAME = "pgjdbc";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be set to something unique (i.e. not clash with the pgjdbc actual login config name)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well we are going to use it here

jaasApplicationName = "pgjdbc";
to make the actual connection. So it seems appropriate to have the same name. I agree we could get away with just checking for the cache, but since we are going to the trouble, we might as well make sure it is going to work

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine, but wanted to note it's a departure of how it used to work before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, I am aware. Which is why I wanted someone else to look over it for sanity. I have to admit to being a bit in the dark so looking for advice from an expert.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert in this area unfortunately, btw.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm... trial by fire then..

Comment on lines 64 to 69
if ( entries == null ) {
jaasApplicationName = PGProperty.JAAS_APPLICATION_NAME.getDefaultValue();
Configuration.setConfiguration(new CustomKrbConfig());
} else {
Configuration.setConfiguration(new CustomKrbConfig());
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks strange. In either case your are setting CustomKrbConfig which only knows about pgjdbc. If somebody sets the jaas.config to, say, foo, then it still handles only pgjdbc. It won't see/use foo. It's unclear what the intent is. If the intent is to only use foo if the config is set (also for the creds-cache-lookup), then this isn't doing it. I think in that case it would fail, since foo is attempted to be looked up, but only pgjdbc will be set.

Since the previous version of this code doesn't do anything with the specified JAAS config, I wonder why do it now?

Shouldn't it be doing something like this (set the login config for the cache check to something other than pgjdbc as it's really doing something else: check the ticket cache, not try to login)?

Configration oldConfig = Configuration.getConfiguration();
Configuration.setConfiguration(new CustomKrbConfig());
lc = new LoginContext("krbTicketCacheCheck", new CallbackHandler() { ....
lc.login();
...
Subject sub = lc.getSubject();
boolean result = sub != null;
Configuration.setConfiguration(oldConfig);
return result;

@jerboaa jerboaa mentioned this pull request Dec 9, 2021
@davecramer
Copy link
Member Author

@jerboaa FYI, this doesn't actually work since it won't find the default principal. Still working on it.

@davecramer davecramer merged commit 06cecbb into pgjdbc:master Dec 20, 2021
@davecramer davecramer deleted the fixkerberos branch December 20, 2021 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants