-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat/gen ssh key #3656
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
base: develop
Are you sure you want to change the base?
feat/gen ssh key #3656
Changes from all commits
2a741b7
186b1b8
3463fbb
1fc0245
aaecddd
1d638a4
e1672e7
a17659f
f12be5c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,10 +82,10 @@ func (d *SqlDb) UpdateAccessKey(key db.AccessKey) error { | |
| } | ||
|
|
||
| if key.OverrideSecret { | ||
|
|
||
| query += ", type=?, secret=?, source_storage_id=?, source_storage_key=?, source_storage_type=?" | ||
| query += ", type=?, secret=?, plain=?, source_storage_id=?, source_storage_key=?, source_storage_type=?" | ||
| args = append(args, key.Type) | ||
| args = append(args, key.Secret) | ||
| args = append(args, key.Plain) | ||
|
cursor[bot] marked this conversation as resolved.
cursor[bot] marked this conversation as resolved.
Comment on lines
+85
to
+88
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
rg -n -C 5 '\b(OverrideSecret|IgnorePlain)\b' --glob '*.go' .
golangci-lint run --timeout=3mRepository: semaphoreui/semaphore Length of output: 9602 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- db/sql/access_key.go ---'
sed -n '66,102p' db/sql/access_key.go
printf '%s\n' '--- update callers ---'
sed -n '105,170p' services/server/access_key_svc.go
sed -n '108,136p' api/projects/environment.go
sed -n '198,225p' services/server/secret_storage_svc.go
sed -n '168,190p' cli/cmd/vault_rekey.go
printf '%s\n' '--- related tests ---'
rg -n -C 4 'UpdateAccessKey|OverrideSecret|IgnorePlain' --glob '*_test.go' db services api cliRepository: semaphoreui/semaphore Length of output: 10143 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import sqlite3
def build_update(override_secret, ignore_plain):
query = "update access_key set name=?"
args = ["new-name"]
if not ignore_plain:
query += ", plain=?"
args.append("ordinary-plain")
if override_secret:
query += ", type=?, secret=?, plain=?, source_storage_id=?, source_storage_key=?, source_storage_type=?"
args.extend(["string", "secret", "override-plain", 1, "key", "vault"])
return query, args
for override_secret in (False, True):
for ignore_plain in (False, True):
query, args = build_update(override_secret, ignore_plain)
conn = sqlite3.connect(":memory:")
conn.execute("""
create table access_key (
name text, plain text, type text, secret text,
source_storage_id integer, source_storage_key text,
source_storage_type text
)
""")
conn.execute("insert into access_key values ('old', 'old-plain', 'old-type', 'old-secret', null, null, null)")
try:
conn.execute(query + " where name=?", args + ["old"])
row = conn.execute("select name, plain, type, secret from access_key").fetchone()
print({
"OverrideSecret": override_secret,
"IgnorePlain": ignore_plain,
"plain_assignments": query.count("plain=?"),
"sqlite_result": row,
})
except Exception as exc:
print({
"OverrideSecret": override_secret,
"IgnorePlain": ignore_plain,
"plain_assignments": query.count("plain=?"),
"sqlite_error": str(exc),
})
PYRepository: semaphoreui/semaphore Length of output: 726 🏁 Script executed: #!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- access-key validation and fields ---'
sed -n '1,125p' db/AccessKey.go
printf '%s\n' '--- secret serialization paths ---'
rg -n -C 8 'func .*SerializeSecret|func .*DeserializeSecret|Plain\s*=|IgnorePlain\s*=' services/server --glob '*.go'
printf '%s\n' '--- supported SQL drivers and dialect setup ---'
rg -n -C 3 'sqlite|postgres|mysql|sql.Open|driver' go.mod go.sum db --glob '*.go' --glob 'go.mod'
printf '%s\n' '--- exact update query tests or SQL mocks ---'
rg -n -C 5 'update access_key set|plain=\?|source_storage_type|UpdateAccessKey' --glob '*_test.go' --glob '*.go' .Repository: semaphoreui/semaphore Length of output: 40147 🌐 Web query:
💡 Result: In PostgreSQL, specifying the same target column more than once in the SET clause of an UPDATE statement is not allowed and will result in an error [1]. Specifically, executing such a command will raise an error stating: ERROR: multiple assignments to same column "column_name" [1]. This behavior occurs because the order of assignments in an UPDATE statement is not significant, and all expressions in the SET clause are evaluated using the "old" (pre-update) values of the row [1]. Since PostgreSQL cannot determine a meaningful order of precedence for conflicting assignments to the same column, it prohibits the duplicate specification entirely [1]. To perform multiple updates on the same column, you must use separate UPDATE statements [2]. Additionally, when referencing columns within an UPDATE, keep in mind that all column references in your expressions refer to the original values of the row at the start of the update operation, not any new values being assigned in the same statement [1]. Citations:
Assign When 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
| args = append(args, key.SourceStorageID) | ||
| args = append(args, key.SourceStorageKey) | ||
| args = append(args, key.SourceStorageType) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,13 @@ | ||
| package server | ||
|
|
||
| import ( | ||
| "bufio" | ||
| "bytes" | ||
| "encoding/json" | ||
| "errors" | ||
|
|
||
| "github.com/semaphoreui/semaphore/db" | ||
| "github.com/semaphoreui/semaphore/util" | ||
| "github.com/semaphoreui/semaphore/pkg/common_errors" | ||
| ) | ||
|
|
||
|
|
@@ -69,7 +73,48 @@ func (s *AccessKeyServiceImpl) GetAll(projectID int, options db.GetAccessKeyOpti | |
| return s.accessKeyRepo.GetAccessKeys(projectID, options, params) | ||
| } | ||
|
|
||
| func maybeGenerateSSHPrivateKey(key *db.AccessKey) error { | ||
| if !key.GenerateSSHKey || key.Type != db.AccessKeySSH { | ||
| key.Plain = nil | ||
| return nil | ||
| } | ||
|
|
||
| var b bytes.Buffer | ||
| privateKeyFile := bufio.NewWriter(&b) | ||
|
|
||
| publicKey, err := util.GeneratePrivateKey(privateKeyFile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| err = privateKeyFile.Flush() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| key.SshKey.PrivateKey = b.String() | ||
|
|
||
| type sshPublicKey struct { | ||
| PublicKey string `json:"public_key"` | ||
| } | ||
|
|
||
| plainBytes, err := json.Marshal(sshPublicKey{ | ||
| PublicKey: publicKey, | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| plain := string(plainBytes) | ||
| key.Plain = &plain | ||
| return nil | ||
| } | ||
|
Comment on lines
+76
to
+111
|
||
|
|
||
| func (s *AccessKeyServiceImpl) Create(key db.AccessKey) (newKey db.AccessKey, err error) { | ||
| err = maybeGenerateSSHPrivateKey(&key) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| // SerializeSecret encrypts/persists the secret for writable backends. For read-only | ||
| // external storage the secret is not stored in Semaphore, so SerializeSecret fails | ||
|
|
@@ -89,6 +134,11 @@ func (s *AccessKeyServiceImpl) Update(key db.AccessKey) (err error) { | |
| return | ||
| } | ||
|
|
||
| err = maybeGenerateSSHPrivateKey(&key) | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| var oldKey db.AccessKey | ||
| oldKey, err = s.accessKeyRepo.GetAccessKey(*key.ProjectID, key.ID) | ||
| if err != nil { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -150,15 +150,49 @@ | |||||||||||||||||||||||||||||||||||||||||||||||
| dense | ||||||||||||||||||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| <v-checkbox | ||||||||||||||||||||||||||||||||||||||||||||||||
| v-model="item.generate_ssh_key" | ||||||||||||||||||||||||||||||||||||||||||||||||
| label="Generate SSH Key" | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
| label="Generate SSH Key" | |
| :label="$t('generateSshKey')" |
Copilot
AI
Feb 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checkbox for generating SSH keys is only disabled when canEditSecrets is false, but it should also be disabled for existing keys where override_secret is required. When editing an existing key without override_secret checked, users can toggle generate_ssh_key, but this won't have any effect because the secret won't be updated. This creates a confusing user experience.
Consider adding || (!isNew && !item.override_secret) to the disabled condition to make it clear that SSH key generation only works when secrets can be edited.
| :disabled="formSaving || !canEditSecrets" | |
| :disabled="formSaving || !canEditSecrets || (!isNew && !item.override_secret)" |
Copilot
AI
Feb 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline styles are used extensively for the public key display section. Consider extracting these styles to a scoped <style> section or using CSS classes for better maintainability and consistency. This makes it easier to maintain and update styling across the application.
| <div style="position: relative"> | |
| <pre | |
| style=" | |
| overflow: auto; | |
| background: gray; | |
| color: white; | |
| border-radius: 10px; | |
| margin-top: 5px; | |
| " | |
| class="pa-2" | |
| >{{ publicKey }}</pre | |
| > | |
| <CopyClipboardButton | |
| style="position: absolute; right: 0; top: 0; transform: scale(0.9);" | |
| <div class="public-key-container"> | |
| <pre | |
| class="pa-2 public-key-display" | |
| >{{ publicKey }}</pre | |
| > | |
| <CopyClipboardButton | |
| class="public-key-copy-button" |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |||||||||||||||||||
| :save-button-text="itemId === 'new' ? $t('create') : $t('save')" | ||||||||||||||||||||
| :title="`${itemId === 'new' ? $t('nnew') : $t('edit')} Key`" | ||||||||||||||||||||
| :max-width="450" | ||||||||||||||||||||
| @save="loadItems()" | ||||||||||||||||||||
| @save="loadItemsAndShowPublicKey($event)" | ||||||||||||||||||||
| > | ||||||||||||||||||||
| <template v-slot:form="{ onSave, onError, needSave, needReset }"> | ||||||||||||||||||||
| <KeyForm | ||||||||||||||||||||
|
|
@@ -20,6 +20,37 @@ | |||||||||||||||||||
| </template> | ||||||||||||||||||||
| </EditDialog> | ||||||||||||||||||||
|
|
||||||||||||||||||||
| <EditDialog | ||||||||||||||||||||
| :max-width="700" | ||||||||||||||||||||
| v-model="createdPublicKeyDialog" | ||||||||||||||||||||
| :save-button-text="null" | ||||||||||||||||||||
| title="Generated SSH Public Key" | ||||||||||||||||||||
|
||||||||||||||||||||
| title="Generated SSH Public Key" | |
| :title="$t('generatedSshPublicKey')" |
Copilot
AI
Feb 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline styles are duplicated between KeyForm.vue and Keys.vue for displaying the public key. The same styling is applied to the <pre> element in both files (gray background, white color, border-radius, etc.). Consider creating a reusable component or shared styles to avoid this duplication and ensure consistency.
| style=" | |
| overflow: auto; | |
| background: gray; | |
| color: white; | |
| border-radius: 10px; | |
| margin-top: 5px; | |
| " | |
| class="pa-2" | |
| class="pa-2 mt-1 rounded overflow-auto grey darken-3 white--text" |
Copilot
AI
Feb 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for determining if a public key should be shown on update (line 156) assumes that if generate_ssh_key is true, a key was generated. However, this flag comes from the request body, not the response. If the backend fails to generate the key (but doesn't return an error), or if the key generation is skipped for some reason, the dialog may still appear with an empty public key. Consider adding an additional check to ensure the public key was actually generated before showing the dialog.
Uh oh!
There was an error while loading. Please reload this page.