feat(mgr): sync user and admin SSH keys via markers, revoke on deactivate
- Write user-managed key lines with a Vpsmgr-User marker and operator keys with a Vpsmgr-Operator marker - applySSHKeysScript now drops every marked line then re-adds the still active/granted set, so unchecking or deleting a key actually removes it from authorized_keys; hand-added (unmarked) keys are never touched - Update tests for the new markers and the same-key-in-both-roles cases
This commit is contained in:
bluebluesoda committed
1 parent
aa3c718522
commit
ed648a6b12
3 files changed
+112
-57
No files matched your search
@@ -131,22 +131,26 @@ func TestApplySSHKeysScriptSync(t *testing.T) {
|
||||
admin := []db.AdminKey{{Key: ed25519Key}}
|
||||
script := applySSHKeysScript(user, admin)
|
||||
|
||||
// Every admin-managed line (carrying the marker) is dropped first, so a
|
||||
// key the user unchecks or the operator deletes no longer lingers.
|
||||
if !strings.Contains(script, "sed -i '/ "+AdminKeyMarker+"$/d' \"$AUTH\"") {
|
||||
t.Errorf("script missing stale-admin-line removal:\n%s", script)
|
||||
// Every panel-managed line is dropped first (both markers), so a key the
|
||||
// user unchecks/deletes no longer lingers.
|
||||
if !strings.Contains(script, "sed -i '/ "+UserKeyMarker+"$/d' \"$AUTH\"") {
|
||||
t.Errorf("script missing user-key removal:\n%s", script)
|
||||
}
|
||||
// The activated admin key is re-appended with the marker.
|
||||
if !strings.Contains(script, "sed -i '/ "+AdminKeyMarker+"$/d' \"$AUTH\"") {
|
||||
t.Errorf("script missing admin-key removal:\n%s", script)
|
||||
}
|
||||
// The activated admin key is re-appended with its marker.
|
||||
wantAdmin := ed25519Key + " " + AdminKeyMarker
|
||||
if !strings.Contains(script, "printf '%s\\n' '"+wantAdmin+"'") {
|
||||
t.Errorf("script missing activated admin key:\n%s", script)
|
||||
}
|
||||
// The user's own key is appended WITHOUT the marker.
|
||||
if !strings.Contains(script, "printf '%s\\n' '"+user[0]+"'") {
|
||||
t.Errorf("script missing user key:\n%s", script)
|
||||
// The user's own active key is re-appended with ITS marker.
|
||||
wantUser := user[0] + " " + UserKeyMarker
|
||||
if !strings.Contains(script, "printf '%s\\n' '"+wantUser+"'") {
|
||||
t.Errorf("script missing user key (marked):\n%s", script)
|
||||
}
|
||||
if strings.Contains(script, "printf '%s\\n' '"+user[0]+" "+AdminKeyMarker) {
|
||||
t.Error("user's own key must not carry the admin marker")
|
||||
if strings.Contains(script, "printf '%s\\n' '"+user[0]+"'") {
|
||||
t.Error("user's own key must carry the user marker (not be unmarked)")
|
||||
}
|
||||
|
||||
// A non-activated admin key (absent from adminKeys) must NOT be appended —
|
||||
@@ -155,22 +159,65 @@ func TestApplySSHKeysScriptSync(t *testing.T) {
|
||||
if strings.Contains(scriptEmpty, ed25519Key) {
|
||||
t.Errorf("script should not append a non-activated admin key:\n%s", scriptEmpty)
|
||||
}
|
||||
// But the sed removal line is still there, so a previously injected copy
|
||||
// But the sed removal lines are still there, so a previously injected copy
|
||||
// of that key gets purged on save.
|
||||
if !strings.Contains(scriptEmpty, "sed -i '/ "+AdminKeyMarker+"$/d'") {
|
||||
t.Errorf("script must still purge stale admin lines:\n%s", scriptEmpty)
|
||||
}
|
||||
}
|
||||
|
||||
// TestAdminKeyMarkerSafe guards the injection-safety invariant: the marker is
|
||||
// embedded inside single quotes in a shell script (see applySSHKeysScript) and
|
||||
// inside a sed address, so it must contain no single quote, backslash, newline
|
||||
// or slash that could break out.
|
||||
func TestAdminKeyMarkerSafe(t *testing.T) {
|
||||
if AdminKeyMarker == "" {
|
||||
t.Fatal("AdminKeyMarker must not be empty")
|
||||
// TestApplySSHKeysScriptSameKeyBothRoles covers the tricky case where the SAME
|
||||
// public key is simultaneously a user's own active key AND a granted admin key.
|
||||
// They are written as two independent marked lines (user marker vs admin
|
||||
// marker), so the key is effective regardless of which role activated it —
|
||||
// there is no ordering bug that could make it ineffective.
|
||||
func TestApplySSHKeysScriptSameKeyBothRoles(t *testing.T) {
|
||||
// Case A: same key is the user's own active key AND a granted admin key.
|
||||
both := applySSHKeysScript([]string{ed25519Key}, []db.AdminKey{{Key: ed25519Key}})
|
||||
if !strings.Contains(both, "printf '%s\\n' '"+ed25519Key+" "+UserKeyMarker+"'") {
|
||||
t.Errorf("user's own copy must carry the user marker:\n%s", both)
|
||||
}
|
||||
if strings.ContainsAny(AdminKeyMarker, "'\\\n/") {
|
||||
t.Errorf("AdminKeyMarker %q contains shell-breaking characters", AdminKeyMarker)
|
||||
if !strings.Contains(both, "printf '%s\\n' '"+ed25519Key+" "+AdminKeyMarker+"'") {
|
||||
t.Errorf("granted admin copy must carry the admin marker:\n%s", both)
|
||||
}
|
||||
|
||||
// Case B: same key only active as the user's OWN key (admin copy not
|
||||
// granted) -> only the user-marked line is appended.
|
||||
ownOnly := applySSHKeysScript([]string{ed25519Key}, nil)
|
||||
if !strings.Contains(ownOnly, "printf '%s\\n' '"+ed25519Key+" "+UserKeyMarker+"'") {
|
||||
t.Errorf("user-active-only key must be appended:\n%s", ownOnly)
|
||||
}
|
||||
if strings.Contains(ownOnly, ed25519Key+" "+AdminKeyMarker) {
|
||||
t.Errorf("user-active-only must not append an admin-marked line:\n%s", ownOnly)
|
||||
}
|
||||
|
||||
// Case C: same key only active as a GRANTED admin key (not in the user's
|
||||
// own list) -> only the admin-marked line is appended; SSH ignores the
|
||||
// comment so it still authorizes the key.
|
||||
grantOnly := applySSHKeysScript(nil, []db.AdminKey{{Key: ed25519Key}})
|
||||
if !strings.Contains(grantOnly, "printf '%s\\n' '"+ed25519Key+" "+AdminKeyMarker+"'") {
|
||||
t.Errorf("grant-only key must be appended marked:\n%s", grantOnly)
|
||||
}
|
||||
if strings.Contains(grantOnly, "printf '%s\\n' '"+ed25519Key+"'") {
|
||||
t.Errorf("grant-only must not append an unmarked user line:\n%s", grantOnly)
|
||||
}
|
||||
}
|
||||
|
||||
// TestKeyMarkersSafe guards the injection-safety invariant: both markers are
|
||||
// embedded inside single quotes in a shell script (see applySSHKeysScript) and
|
||||
// inside sed addresses, so each must contain no single quote, backslash,
|
||||
// newline or slash that could break out. They must also be distinct (and not
|
||||
// end-of-line equivalent) so purging one never removes the other.
|
||||
func TestKeyMarkersSafe(t *testing.T) {
|
||||
for _, m := range []string{UserKeyMarker, AdminKeyMarker} {
|
||||
if m == "" {
|
||||
t.Fatal("markers must not be empty")
|
||||
}
|
||||
if strings.ContainsAny(m, "'\\\n/") {
|
||||
t.Errorf("marker %q contains shell-breaking characters", m)
|
||||
}
|
||||
}
|
||||
if UserKeyMarker == AdminKeyMarker {
|
||||
t.Fatal("user and admin markers must be distinct")
|
||||
}
|
||||
}
|
||||
+38
-31
@@ -27,14 +27,18 @@ func ParsePublicKey(s string) (clean, comment string, ok bool) {
|
||||
return m[1] + " " + m[2], strings.TrimSpace(m[3]), true
|
||||
}
|
||||
|
||||
// AdminKeyMarker is appended as a trailing comment to an operator key line when
|
||||
// it is written into a user's ~/.ssh/authorized_keys. SSH ignores anything
|
||||
// after the key body, so it never affects login; it exists so the user can
|
||||
// recognize and find admin-injected keys on their machine even after the
|
||||
// operator deletes the key from the panel (the DB grant is gone, but the
|
||||
// authorized_keys line remains). It is a plain ASCII token — never localized —
|
||||
// so the file stays clean and re-applies deduplicate reliably.
|
||||
const AdminKeyMarker = "from-admin"
|
||||
// UserKeyMarker is appended as a trailing comment to a user's own panel key
|
||||
// line written into ~/.ssh/authorized_keys. SSH ignores anything after the key
|
||||
// body, so it never affects login; it lets the panel identify and remove the
|
||||
// lines it manages (so unchecking/deleting a key actually revokes it) without
|
||||
// ever touching keys the user added by hand. A single ASCII token (no spaces)
|
||||
// is easy to grep and safe to embed in the sync script.
|
||||
const UserKeyMarker = "Vpsmgr-User"
|
||||
|
||||
// AdminKeyMarker is the marker for operator keys the user has activated. Like
|
||||
// UserKeyMarker it is a fixed ASCII comment, so the panel can purge a revoked
|
||||
// admin key line even after forgetting its content.
|
||||
const AdminKeyMarker = "Vpsmgr-Operator"
|
||||
|
||||
// SSHKeyInput is one key row submitted by the panel. ID > 0 updates an
|
||||
// existing key; ID == 0 adds a new one.
|
||||
@@ -225,21 +229,21 @@ func (m *Manager) GrantedAdminKeys(name string) ([]db.AdminKey, error) {
|
||||
return m.db.GrantedAdminKeys(u.ID)
|
||||
}
|
||||
|
||||
// ApplySSHKeys syncs the container's ~/.ssh/authorized_keys. The two kinds of
|
||||
// keys behave differently, deliberately:
|
||||
// ApplySSHKeys syncs the container's ~/.ssh/authorized_keys. Every line the
|
||||
// panel writes carries a marker — the user's own active keys use UserKeyMarker,
|
||||
// the operator keys the user activated use AdminKeyMarker — so the whole
|
||||
// panel-managed set can be dropped and rebuilt on every save. Consequently:
|
||||
//
|
||||
// - adminKeys (the operator keys this user activated) are FULLY SYNCED. Every
|
||||
// line carrying the AdminKeyMarker is dropped, then the activated keys are
|
||||
// re-appended with the marker. So an admin key the user unchecks — or one
|
||||
// the operator has since deleted — is removed from the machine, while an
|
||||
// activated one is present exactly once. Stale admin lines never linger.
|
||||
// - userKeys (the user's own active panel keys) are APPEND-only: added if
|
||||
// missing, never removed. A key the user added by hand and never put in the
|
||||
// panel is untouched, because there is no marker to identify it.
|
||||
// - a key the user unchecks or deletes (own key) or revokes (admin grant) is
|
||||
// removed from the machine, because its marked line is dropped and not
|
||||
// re-added; and
|
||||
// - a key the user added by hand and never put in the panel has no marker, so
|
||||
// it is never touched.
|
||||
//
|
||||
// The script embeds each key in single quotes; ParsePublicKey guarantees the
|
||||
// stored keys contain only the type name, a space and base64 characters, and
|
||||
// AdminKeyMarker is a fixed ASCII token, so no quoting can be escaped.
|
||||
// both markers are fixed ASCII comments (no quote/backslash/slash/newline), so
|
||||
// no quoting can be escaped.
|
||||
func (m *Manager) ApplySSHKeys(name string, userKeys []string, adminKeys []db.AdminKey) error {
|
||||
_, err := m.lx.ExecSH(name, applySSHKeysScript(userKeys, adminKeys))
|
||||
return err
|
||||
@@ -255,10 +259,22 @@ func applySSHKeysScript(userKeys []string, adminKeys []db.AdminKey) string {
|
||||
b.WriteString("AUTH=/root/.ssh/authorized_keys\n")
|
||||
b.WriteString("touch \"$AUTH\"\n")
|
||||
b.WriteString("chmod 600 \"$AUTH\"\n")
|
||||
// Drop every admin-managed line (those ending with the marker) first, then
|
||||
// re-add the ones that are still activated. This makes admin keys a clean
|
||||
// sync: unactivated/deleted ones are removed, activated ones present once.
|
||||
// Drop every panel-managed line first (the user's own keys and the
|
||||
// operator's keys both carry a marker), then re-add the ones that are still
|
||||
// active/granted. A deactivated or deleted key is thus removed; keys added
|
||||
// by hand (no marker) are never touched.
|
||||
b.WriteString("sed -i '/ " + UserKeyMarker + "$/d' \"$AUTH\"\n")
|
||||
b.WriteString("sed -i '/ " + AdminKeyMarker + "$/d' \"$AUTH\"\n")
|
||||
// Re-add the user's own active keys with their marker.
|
||||
for _, k := range userKeys {
|
||||
line := k + " " + UserKeyMarker
|
||||
b.WriteString("grep -qxF -- '")
|
||||
b.WriteString(line)
|
||||
b.WriteString("' \"$AUTH\" 2>/dev/null || printf '%s\\n' '")
|
||||
b.WriteString(line)
|
||||
b.WriteString("' >> \"$AUTH\"\n")
|
||||
}
|
||||
// Re-add the granted admin keys with their marker.
|
||||
for _, k := range adminKeys {
|
||||
line := k.Key + " " + AdminKeyMarker
|
||||
b.WriteString("grep -qxF -- '")
|
||||
@@ -267,14 +283,5 @@ func applySSHKeysScript(userKeys []string, adminKeys []db.AdminKey) string {
|
||||
b.WriteString(line)
|
||||
b.WriteString("' >> \"$AUTH\"\n")
|
||||
}
|
||||
// The user's own active keys are append-only (added if missing, never
|
||||
// removed).
|
||||
for _, k := range userKeys {
|
||||
b.WriteString("grep -qxF -- '")
|
||||
b.WriteString(k)
|
||||
b.WriteString("' \"$AUTH\" 2>/dev/null || printf '%s\\n' '")
|
||||
b.WriteString(k)
|
||||
b.WriteString("' >> \"$AUTH\"\n")
|
||||
}
|
||||
return b.String()
|
||||
}
|
||||
@@ -504,12 +504,13 @@ func (s *Server) handleSSHKeys(w http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
_ = s.db.AddAuditLog(s.auditActor(r, u.Name), "ssh_keys")
|
||||
// Apply the selection immediately. Admin keys are fully synced (activated
|
||||
// ones written with the AdminKeyMarker, stale/unactivated ones removed), so
|
||||
// this runs even when nothing is active — deactivating an admin key must
|
||||
// purge it from the machine. The user's own keys stay append-only. Best-
|
||||
// effort: keys persist in the DB either way, so a save must not fail just
|
||||
// because the container is stopped or unreachable.
|
||||
// Apply the selection immediately. Both the user's own keys and the
|
||||
// operator's keys are fully synced by marker (activated ones written with
|
||||
// their marker, stale/deactivated ones removed), so this runs even when
|
||||
// nothing is active — deactivating a key must purge it from the machine.
|
||||
// Keys the user added by hand (no marker) are never touched. Best-effort:
|
||||
// keys persist in the DB either way, so a save must not fail just because
|
||||
// the container is stopped or unreachable.
|
||||
warn := ""
|
||||
var userActive []string
|
||||
for _, k := range keys {
|
||||
|
||||
Reference in new issue
Block a user