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

Add instance-level secrets #27725

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jbgomond
Copy link
Contributor

@jbgomond jbgomond commented Oct 21, 2023

This PR adds instance-level secrets, and so closes #27373.

I did the implementation next to the current secrets code. I was wondering though if it was intentional that the code was outside the action files or legacy ? (it's out of the scope of this PR, but I was wondering)

image

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 21, 2023
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 21, 2023
@github-actions github-actions bot added modifies/translation modifies/api This PR adds API routes or modifies them modifies/migrations labels Oct 21, 2023
@go-gitea go-gitea deleted a comment from GiteaBot Oct 22, 2023
models/secret/secret.go Outdated Show resolved Hide resolved
routers/api/v1/api.go Outdated Show resolved Hide resolved
@jbgomond jbgomond force-pushed the feature/instance-secrets branch 2 times, most recently from 88ef9f1 to 15d6d7c Compare October 29, 2023 10:02
@jbgomond jbgomond changed the title Added instance-level secrets Add instance-level secrets Oct 29, 2023
KN4CK3R pushed a commit that referenced this pull request Oct 29, 2023
Fixed a little mistake when you deleting user secrets via the API. Found
it when working on #27725.
It should be backported to 1.21 I think.
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Oct 29, 2023
Fixed a little mistake when you deleting user secrets via the API. Found
it when working on go-gitea#27725.
It should be backported to 1.21 I think.
lunny pushed a commit that referenced this pull request Oct 29, 2023
Backport #27829 by @jbgomond

Fixed a little mistake when you deleting user secrets via the API. Found
it when working on #27725.
It should be backported to 1.21 I think.

Co-authored-by: Jean-Baptiste Gomond <dev@jbgomond.com>
@lunny lunny added this to the 1.22.0 milestone Oct 29, 2023
routers/api/v1/api.go Outdated Show resolved Hide resolved
@jbgomond
Copy link
Contributor Author

jbgomond commented Nov 9, 2023

Is it ok to be merged ?

@lunny
Copy link
Member

lunny commented Jan 30, 2024

@go-gitea/technical-oversight-committee

Copy link
Member

@wolfogre wolfogre left a comment

Choose a reason for hiding this comment

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

I hope we can add a warning when adding a global secret in the UI. Like

"
Although secrets will be masked if users try to print them in Actions workflows, this is not absolutely secure. Users can still obtain the contents of secrets by writing malicious workflows, so please ensure that global secrets are not used by people you do not trust. Otherwise, please use organization/user-level or repository-level secrets to limit their scope of use. Alternatively, if it's acceptable to expose their contents, please use global variables.
"

Or it will be terrible if someone adds a global secret to a public instance and thinks it will be safe just because it's labeled as "secret".

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 31, 2024
@delvh
Copy link
Member

delvh commented Feb 9, 2024

@jbgomond will you add the change, or should someone else take over?

@jbgomond
Copy link
Contributor Author

No pb, I'll do it tomorrow

@jbgomond
Copy link
Contributor Author

Is it good like that ?

image

Copy link
Member

@KN4CK3R KN4CK3R left a comment

Choose a reason for hiding this comment

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

I see two problems:

  • For public instances this a Anti-Feature. Even for closed instances, org secrets should be enough
  • No one knows about possible existing global secrets. But we have this problem already with org/user secrets.

Comment on lines +20 to +21
{{if .ctxData.Secrets}}
{{range .ctxData.Secrets}}
Copy link
Member

Choose a reason for hiding this comment

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

Not needed, {{range }} supports {{else}}.

@delvh
Copy link
Member

delvh commented Feb 16, 2024

Wait, we already have instance level variables.
As mentioned by everyone so far, what is the usecase for instance wide secrets?
Secrets are meant to stay secret by design.
However, instance-wide secrets are not secret at all as everyone can print value.
As such, it sounds like a really bad idea to me.
Or am I missing something?

silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
Fixed a little mistake when you deleting user secrets via the API. Found
it when working on go-gitea#27725.
It should be backported to 1.21 I think.
@GiteaBot
Copy link
Contributor

GiteaBot commented Mar 2, 2024

This pull request has a last call and has not had any activity in the past two weeks. Consider it to be a polite refusal. 🍵

@GiteaBot GiteaBot closed this Mar 2, 2024
@GiteaBot GiteaBot removed this from the 1.22.0 milestone Mar 2, 2024
@techknowlogick techknowlogick reopened this Mar 2, 2024
@techknowlogick
Copy link
Member

techknowlogick commented Mar 2, 2024

Reopening.

While we have instance wide variables, having instance wide secrets would be useful for a specific use case, if the instance is targeted to specific user group then this could work. As it is possible to extract secrets through various ways if you have access the be able to run a workflow, so it wouldnt work to protect a secret on a public instance or on one with untrusted users.

Previously when I used drone I did something like this to store docker hub secrets as I had repos in different orgs, but they all needed to push to the same place. And having this allowed me not to have to duplicate a secret. This only worked as I was the only one on the instance.

The above is to say I am welcoming of this PR, and would love to see it get in, I’m thinking it just needs some guide rails on it to ensure when people use it they are aware of the caveats.

@lunny
Copy link
Member

lunny commented Mar 2, 2024

per #27725 (review), I think maybe we can close this one.

@jbgomond
Copy link
Contributor Author

jbgomond commented Mar 3, 2024

For public instances this a Anti-Feature. Even for closed instances, org secrets should be enough

I disagree on that one. For public instances sure, it's useless. But Gitea is used by small firms or groups to have a private Git solution, and projects are separated in different "organisations", as are "workspaces" in Bitbucket, to separate the clients / projects.

In that type of situation (it's my case), we have single Docker instance, and we need to store passwords across all ""organisations"" because it's the same account. Using variables is not the solution, because they are shown plain text in the action logs.

Without instance level secrets, the only solution is to duplicate secrets in all organisations, and in case of changes, to update all of them. It's not feasable, I have more than 100 organisations (projects) created.

I understand that the secret system is flawed at its core, but in that case, it is the same with organisation secrets. Maybe there's room to improve that then ? Also, if we are scared about public instances, maybe a startup option would be the solution ? So that administrators are voluntarily enabling the feature ?

@techknowlogick
Copy link
Member

@jbgomond I think we pretty much have the same use case for this.

@lunny
Copy link
Member

lunny commented Mar 4, 2024

Maybe we can have a configuration item so users can chose enable/disable it?

@jbgomond
Copy link
Contributor Author

jbgomond commented Mar 6, 2024

Fine for me, a good intermediate solution

@GiteaBot

This comment was marked as resolved.

@GiteaBot GiteaBot closed this Mar 22, 2024
@lunny lunny reopened this Mar 22, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files labels Mar 22, 2024
@delvh delvh removed the pr/last-call This PR has been stale for too long. If no one reviewes it within a week, it will be closed. label Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/translation size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instance-level secrets
7 participants