fix: SFTP auth bypass with no pub key in LDAP (#20986)

If a user attempts to authenticate with a key but does not have an
sshpubkey attribute in LDAP, the server allows the connection, which 
means the server trusted the key without reason. This is now fixed, 
and a test has been added for validation.
This commit is contained in:
Aditya Manthramurthy
2025-02-27 10:43:32 -08:00
committed by GitHub
parent 6cd8a372cb
commit 4c71f1b4ec
2 changed files with 85 additions and 25 deletions
+68 -17
View File
@@ -91,6 +91,7 @@ func TestSFTPAuthentication(t *testing.T) {
suite.SFTPPublicKeyAuthentication(c)
suite.SFTPFailedPublicKeyAuthenticationInvalidKey(c)
suite.SFTPPublicKeyAuthNoPubKey(c)
suite.TearDownSuite(c)
},
@@ -146,6 +147,32 @@ func (s *TestSuiteIAM) SFTPPublicKeyAuthentication(c *check) {
}
}
// A user without an sshpubkey attribute in LDAP (here: fahim) should not be
// able to authenticate.
func (s *TestSuiteIAM) SFTPPublicKeyAuthNoPubKey(c *check) {
keyBytes, err := os.ReadFile("./testdata/dillon_test_key.pub")
if err != nil {
c.Fatalf("could not read test key file: %s", err)
}
testKey, _, _, _, err := ssh.ParseAuthorizedKey(keyBytes)
if err != nil {
c.Fatalf("could not parse test key file: %s", err)
}
newSSHCon := newSSHConnMock("fahim=ldap")
_, err = sshPubKeyAuth(newSSHCon, testKey)
if err == nil {
c.Fatalf("expected error but got none")
}
newSSHCon = newSSHConnMock("fahim")
_, err = sshPubKeyAuth(newSSHCon, testKey)
if err == nil {
c.Fatalf("expected error but got none")
}
}
func (s *TestSuiteIAM) SFTPFailedAuthDueToMissingPolicy(c *check) {
newSSHCon := newSSHConnMock("dillon=ldap")
_, err := sshPasswordAuth(newSSHCon, []byte("dillon"))
@@ -275,24 +302,48 @@ func (s *TestSuiteIAM) SFTPValidLDAPLoginWithPassword(c *check) {
c.Fatalf("policy add error: %v", err)
}
userDN := "uid=dillon,ou=people,ou=swengg,dc=min,dc=io"
userReq := madmin.PolicyAssociationReq{
Policies: []string{policy},
User: userDN,
}
if _, err := s.adm.AttachPolicy(ctx, userReq); err != nil {
c.Fatalf("Unable to attach policy: %v", err)
}
{
userDN := "uid=dillon,ou=people,ou=swengg,dc=min,dc=io"
userReq := madmin.PolicyAssociationReq{
Policies: []string{policy},
User: userDN,
}
if _, err := s.adm.AttachPolicyLDAP(ctx, userReq); err != nil {
c.Fatalf("Unable to attach policy: %v", err)
}
newSSHCon := newSSHConnMock("dillon=ldap")
_, err = sshPasswordAuth(newSSHCon, []byte("dillon"))
if err != nil {
c.Fatal("Password authentication failed for user (dillon):", err)
}
newSSHCon := newSSHConnMock("dillon=ldap")
_, err = sshPasswordAuth(newSSHCon, []byte("dillon"))
if err != nil {
c.Fatal("Password authentication failed for user (dillon):", err)
}
newSSHCon = newSSHConnMock("dillon")
_, err = sshPasswordAuth(newSSHCon, []byte("dillon"))
if err != nil {
c.Fatal("Password authentication failed for user (dillon):", err)
newSSHCon = newSSHConnMock("dillon")
_, err = sshPasswordAuth(newSSHCon, []byte("dillon"))
if err != nil {
c.Fatal("Password authentication failed for user (dillon):", err)
}
}
{
userDN := "uid=fahim,ou=people,ou=swengg,dc=min,dc=io"
userReq := madmin.PolicyAssociationReq{
Policies: []string{policy},
User: userDN,
}
if _, err := s.adm.AttachPolicyLDAP(ctx, userReq); err != nil {
c.Fatalf("Unable to attach policy: %v", err)
}
newSSHCon := newSSHConnMock("fahim=ldap")
_, err = sshPasswordAuth(newSSHCon, []byte("fahim"))
if err != nil {
c.Fatal("Password authentication failed for user (fahim):", err)
}
newSSHCon = newSSHConnMock("fahim")
_, err = sshPasswordAuth(newSSHCon, []byte("fahim"))
if err != nil {
c.Fatal("Password authentication failed for user (fahim):", err)
}
}
}