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

AuthGssApiWithMic: Use default client creds instead of remote username #743

Merged
merged 6 commits into from
Aug 8, 2022

Conversation

geofft
Copy link
Contributor

@geofft geofft commented Nov 12, 2021

Previously, AuthGssApiWithMic used params.getUsername() to create the
local client credential object. However, at least when using the native
GSS libraries (sun.security.jgss.native=true), the username would need
to be something like "user@EXAMPLE.COM", not "user", or the library is
unable to find credentials. Also, your remote username might not be your
local username.

Instead, and more simply, call the GSSManager#createCredential variant
that just uses default credentials, which should handle both of these
cases.

Tested on Windows using SSPI. I haven't tested this patch on Linux but I
have confirmed that this form of call to createCredential works as I
expect when using the native GSS/Kerberos library there too.

Previously, AuthGssApiWithMic used params.getUsername() to create the
local client credential object. However, at least when using the native
GSS libraries (sun.security.jgss.native=true), the username would need 
to be something like "user@EXAMPLE.COM", not "user", or the library is 
unable to find credentials. Also, your remote username might not be your
local username.

Instead, and more simply, call the GSSManager#createCredential variant
that just uses default credentials, which should handle both of these 
cases.

Tested on Windows using SSPI. I haven't tested this patch on Linux but I
have confirmed that this form of call to createCredential works as I 
expect when using the native GSS/Kerberos library there too.
@vladimirlagunov
Copy link
Contributor

I've tested the patch on macOS with krb5-kdc launched in a container, gssapi authentication seems to work.

@advancedxy
Copy link

This is definitely an improvement. I encountered this issue when configuring ssh configurations on IntelliJ IDEA. The remote username is not the same as my local computer's login name and also not the same with $my_name@EXAMPLE.COM.

I'd love to see this get merged.

@hierynomus
Copy link
Owner

Not sure, but shouldn't we support both types, so with passing a username, and without passing it. Merging this, we exclude the possibility of passing the username directly.

@geofft
Copy link
Contributor Author

geofft commented Dec 16, 2021

This is about the username used to look up the local credentials, not the username used to connect to the remote machine.

One way to think about it is like the names on SSH keypairs. Usually my SSH key has a name like geofft@workstation.example.com. That is my key and identifies me (the client), whatever account I'm logging into. It isn't only for cases where the remote username is geofft (and it definitely isn't only for cases where the remote username is geofft@workstation.example.com).

The current behavior is as if, when logging into root@server.example.com, the client looked for an SSH private key whose name was root, and failed to use public-key auth if it couldn't find a key with such a name. First, the key is probably not named root. Second, the key's name usually has an @... in it.

I can see an argument for an additional feature where you can pass a local credential name as a customizable string (analogous to how you can use a non-default private key), but that's not how the code works right now. Right now it's a non-customizable string, and it's a string that happens to work in almost no cases.

@hierynomus
Copy link
Owner

Is it possible to set up some docker based integration test to test the authgss? Though I believe that it will work, it would be beneficial to have some test added :)

@geofft
Copy link
Contributor Author

geofft commented Dec 30, 2021

Yes, that seems like a good idea. Do you already have an integration test framework or should I try to throw something together with GitHub Actions?

@hierynomus
Copy link
Owner

If you look at the src/itest dir, there's a bunch of Docker based integration tests in there. So adding some test case with configuration for the docker image should be possible without too much strain :)

@geofft
Copy link
Contributor Author

geofft commented Dec 31, 2021

Mostly for my own reference, here's a self-contained example of doing Kerberized SSH (with OpenSSH as both client and server) on docker run -it alpine:latest:

apk add krb5 krb5-server openssh-client-krb5 openssh-server-krb5

# Configure the Kerberos library for a realm named INTEGRATION.TEST running on localhost
cat >> /etc/krb5.conf << EOF
[libdefaults]
default_realm = INTEGRATION.TEST
[realms]
INTEGRATION.TEST = {
  kdc = localhost
}
EOF

# Create the KDC database with master password "password"
kdb5_util create -s -P password

# Create root@INTEGRATION.TEST, also with password "password"
kadmin.local add_principal -pw password root

# Create host keytab in /etc/krb5.keytab
# ktadd will rekey so we can do -nokey when creating it
kadmin.local add_principal -nokey host/"$(hostname -f)"
kadmin.local ktadd host/"$(hostname -f)"

# Start KDC (daemonizes)
krb5kdc

# Start sshd (also daemonizes)
ssh-keygen -f /etc/ssh/ssh_host_rsa_key -N ''
sed -i 's/.*GSSAPIAuthentication.*/GSSAPIAuthentication yes/' /etc/ssh/sshd_config
/usr/sbin/sshd.krb5

# Unlock the root account for ssh
sed -i 's/^root:!:/root:*:/' /etc/shadow

# Log in
echo password | kinit
ssh -K -o BatchMode=yes -o StrictHostKeyChecking=no root@"$(hostname -f)" echo hello

By default, Kerberos lets a user with name X in the default realm log into UNIX account X. You can customize/override this with a ~/.k5login file listing principals, akin to an authorized_keys file. For an example where the Kerberos username doesn't match the UNIX username, try

kadmin.local add_principal -pw password hierynomus
echo password | kinit hierynomus
echo hierynomus@INTEGRATION.TEST >> ~/.k5login
ssh -K -o BatchMode=yes -o StrictHostKeyChecking=no root@"$(hostname -f)" echo hello

I'll give a shot at working this into the integration test framework.

@geofft
Copy link
Contributor Author

geofft commented Jan 6, 2022

Hmmm. So, I have a test (mostly), but it succeeds without this patch. I'm a little worried that what's going on is this patch is necessary on Windows but not Linux.

I too am running into this with IntelliJ IDEA on Windows, and when I said I tested it, I meant that dropping a patched sshj into IntelliJ's classpath works. So there is a little bit of a complication in that IntelliJ's code that creates an SSHClient and calls AuthGssApiWithMic is not visible to me, and in particular, I have no idea how they're getting a LoginContext.

I think my next step is to get the tests running on Windows and see if my test passes. (Or try IntelliJ on Linux?)

@geofft
Copy link
Contributor Author

geofft commented Jan 6, 2022

Actually I take that back, I can reproduce the "client user and server user are different" bug and this patch does fix it on Linux too. I just can't reproduce the subtler "the usernames are the same, but you need the Kerberos realm to look up the credentials" one.

@vladimirlagunov
Copy link
Contributor

I have no idea how they're getting a LoginContext

val loginContext = LoginContext("SshjJaas", null, null, 
  AccessController.doPrivileged(PrivilegedAction { javax.security.auth.login.Configuration.getConfiguration() }))
return AuthGssApiWithMic(loginContext, listOf(Oid("1.2.840.113554.1.2.2")))
SshjJaas {
  com.sun.security.auth.module.Krb5LoginModule required;
};

@codecov-commenter
Copy link

Codecov Report

Merging #743 (3fbb015) into master (69812e9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #743   +/-   ##
=========================================
  Coverage     64.45%   64.46%           
  Complexity     1460     1460           
=========================================
  Files           211      211           
  Lines          8617     8616    -1     
  Branches        807      807           
=========================================
  Hits           5554     5554           
+ Misses         2640     2639    -1     
  Partials        423      423           
Impacted Files Coverage Δ
...chmizz/sshj/userauth/method/AuthGssApiWithMic.java 68.67% <100.00%> (-0.38%) ⬇️
...main/java/net/schmizz/sshj/xfer/scp/SCPEngine.java 73.01% <0.00%> (-3.18%) ⬇️
...zz/sshj/connection/channel/ChannelInputStream.java 87.69% <0.00%> (+1.53%) ⬆️
...t/schmizz/sshj/connection/ConnectionException.java 44.44% <0.00%> (+11.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69812e9...3fbb015. Read the comment docs.

@vladimirlagunov
Copy link
Contributor

JFI, the fix from this PR helped one of our users. I think, I'll backport the patch to intellij, and cover it into a registry flag, allowing to disable (or enable) this code in case of problems.

@geofft
Copy link
Contributor Author

geofft commented Apr 26, 2022

Awesome, thanks, IntelliJ is indeed our use case. :) I'm still trying to find time to get the devcontainers setup fully working, hopefully I'll have time for that in the next few weeks.

@vladimirlagunov
Copy link
Contributor

@geofft My bad, I've just remembered that I forgot to mention it here. I added your change to IDEA. Here's an instruction how to enable it.

@lordmauve
Copy link

@vladimirlagunov Does that mean it's also in JetBrains Gateway, or is that independent?

@vladimirlagunov
Copy link
Contributor

@lordmauve yes, Gateway uses SSHJ under the hood.

@hierynomus
Copy link
Owner

Going to merge this in....

@hierynomus hierynomus merged commit 1b258f0 into hierynomus:master Aug 8, 2022
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.

None yet

6 participants