-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 Gitea secrets storage and management #14483
Changes from 16 commits
6b24ada
47c472a
64a91b4
9f2d204
368c97e
d39f7c4
9465c13
eb2b139
f4df973
abde871
b1e8915
a077ee2
5b1044a
834c7c1
8e1291d
1788de5
0a3be05
a0ebf78
319da15
b08114b
073656c
f89bd80
850d936
41310a7
e54785a
ccb57c8
e30f532
29e4f6b
b01b2a9
a1aee64
4c8f590
bc999bd
41e9be0
2c7ae0c
dd84d07
f5effc1
c08fc15
44ca6bf
6fcb7bf
b79b156
acc0c12
c754525
eb5bcec
3183368
e86e30f
e6cee41
641d37a
7c82f7a
f9d58d4
aa10928
f738069
5103f1d
a23241f
9f8fdaa
b32bb7a
23dd7a7
a8c192d
f1ef5ae
4a2676e
5aa55fe
d1a729b
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 |
---|---|---|
@@ -0,0 +1,55 @@ | ||
// Copyright 2022 The Gitea Authors. All rights reserved. | ||
// Use of this source code is governed by a MIT-style | ||
// license that can be found in the LICENSE file. | ||
|
||
package auth | ||
|
||
import ( | ||
"fmt" | ||
"regexp" | ||
|
||
"code.gitea.io/gitea/modules/timeutil" | ||
) | ||
|
||
type ErrSecretNameInvalid struct { | ||
Name string | ||
} | ||
|
||
func (err ErrSecretNameInvalid) Error() string { | ||
return fmt.Sprintf("secret name %s is invalid", err.Name) | ||
} | ||
|
||
type ErrSecretDataInvalid struct { | ||
Data string | ||
} | ||
|
||
func (err ErrSecretDataInvalid) Error() string { | ||
return fmt.Sprintf("secret data %s is invalid", err.Data) | ||
} | ||
lunny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
var nameRE = regexp.MustCompile("[^a-zA-Z0-9-_.]+") | ||
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. Should we perhaps allow 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. I think What GitHub does:
|
||
|
||
// Secret represents a secret | ||
type Secret struct { | ||
ID int64 | ||
lunny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
UserID int64 `xorm:"index NOTNULL"` | ||
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.
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. Solved. |
||
RepoID int64 `xorm:"index NOTNULL"` | ||
Name string `xorm:"NOTNULL"` | ||
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. I just noticed: If we change our preconditions slightly, we can also do something more advanced: Let's test that there are no drawbacks on both possible values: Assume we have a secret
the only thing to keep in mind when collecting the secrets is that we need to query the org secrets first so that the repo secrets will override the org secrets. One other thing I noticed: We should also delete repo secrets when a repo is deleted and org secrets when an org is deleted. 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. Could it be that the suggestions in this file were missed? 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.
I agree, but I think your example is incorrect. The repo secret for a specific repo, i.e.
Agree. 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. Solved. 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. I think you are right, but I'm not completely sure.
) |
||
Data string `xorm:"TEXT"` | ||
lunny marked this conversation as resolved.
Show resolved
Hide resolved
|
||
PullRequest bool `xorm:"NOTNULL"` | ||
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. I am not quite sure what this attribute is supposed to be: 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. This column indicates whether a pull request could read the secret. I think we could have two types 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. We may use secrets outside of the git scope. A 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. I agree with the However, I disagree with the 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. I think we don't need to add type Secret struct {
// ...
ReadByPullRequest bool
ReadByExternalAPI bool // for example
ReadByXXX bool // then add as needed
} 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. Hey, let's copy someone's homework, maybe the field is unnecessary. |
||
CreatedUnix timeutil.TimeStamp `xorm:"created NOTNULL"` | ||
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. Should we also store 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. I don't think so, we can only add or remove a secret, so there's no "updated time". |
||
} | ||
|
||
// Validate validates the required fields and formats. | ||
func (s *Secret) Validate() error { | ||
switch { | ||
case len(s.Name) == 0: | ||
return ErrSecretNameInvalid{Name: s.Name} | ||
case len(s.Data) == 0: | ||
return ErrSecretDataInvalid{Data: s.Data} | ||
case nameRE.MatchString(s.Name): | ||
return ErrSecretNameInvalid{Name: s.Name} | ||
default: | ||
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. and given my suggestion above, the case 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. Why? If it's a repo secret, the OwnerID(aka UserID) should be 0. So the secret has nothing to do with the repo's owner, and works well with repo transferring. 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. Yep, since we don't need to change our assumptions anymore, that's not needed. |
||
return nil | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
// Copyright 2022 The Gitea Authors. All rights reserved. | ||
// Use of this source code is governed by a MIT-style | ||
// license that can be found in the LICENSE file. | ||
|
||
package migrations | ||
|
||
import ( | ||
"code.gitea.io/gitea/modules/timeutil" | ||
|
||
"xorm.io/xorm" | ||
) | ||
|
||
func createSecretsTable(x *xorm.Engine) error { | ||
type Secret struct { | ||
ID int64 | ||
UserID int64 `xorm:"index NOTNULL"` | ||
RepoID int64 `xorm:"index NOTNULL"` | ||
Name string `xorm:"NOTNULL"` | ||
Data string `xorm:"TEXT"` | ||
PullRequest bool `xorm:"NOTNULL"` | ||
CreatedUnix timeutil.TimeStamp `xorm:"created NOTNULL"` | ||
} | ||
|
||
return x.Sync(new(Secret)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,3 +67,8 @@ func NewSecretKey() (string, error) { | |
|
||
return secretKey, nil | ||
} | ||
|
||
// NewMasterKey generate a new value intended to be used by MASTER_KEY. | ||
func NewMasterKey() ([]byte, error) { | ||
return util.CryptoRandomBytes(32) | ||
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. Why so short when the 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. Solved. Actually, I don't know why it was 32, but I think you're right and it makes sense to have the same length as |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -214,6 +214,8 @@ var ( | |
HMACKey string `ini:"HMAC_KEY"` | ||
Allways bool | ||
}{} | ||
MasterKeyProvider string | ||
MasterKey []byte | ||
Comment on lines
+219
to
+220
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. Those two config options are currently missing from |
||
|
||
// UI settings | ||
UI = struct { | ||
|
@@ -954,6 +956,20 @@ func loadFromConf(allowEmpty bool, extraConfig string) { | |
PasswordCheckPwn = sec.Key("PASSWORD_CHECK_PWN").MustBool(false) | ||
SuccessfulTokensCacheSize = sec.Key("SUCCESSFUL_TOKENS_CACHE_SIZE").MustInt(20) | ||
|
||
// Master key provider configuration | ||
MasterKeyProvider = sec.Key("MASTER_KEY_PROVIDER").MustString("none") | ||
switch MasterKeyProvider { | ||
case "plain": | ||
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. Why plain? |
||
if MasterKey, err = base64.StdEncoding.DecodeString(sec.Key("MASTER_KEY").MustString("")); err != nil { | ||
log.Fatal("error loading master key: %v", err) | ||
return | ||
} | ||
case "none": | ||
default: | ||
log.Fatal("invalid master key provider type: %v", MasterKeyProvider) | ||
return | ||
} | ||
|
||
InternalToken = loadSecret(sec, "INTERNAL_TOKEN_URI", "INTERNAL_TOKEN") | ||
|
||
cfgdata := sec.Key("PASSWORD_COMPLEXITY").Strings(",") | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -176,6 +176,12 @@ app_url_helper = Base address for HTTP(S) clone URLs and email notifications. | |||||
log_root_path = Log Path | ||||||
log_root_path_helper = Log files will be written to this directory. | ||||||
|
||||||
security_title = Security Settings | ||||||
master_key_provider = Master Key Provider | ||||||
master_key_provider_none = None | ||||||
master_key_provider_plain = Plain | ||||||
master_key_provider_helper = Master Key Provider to use to store secret key that will be used for other secret encryption. Use "None" to not encrypt secrets. Use "Plain" to store automatically generated secret in configuration file. | ||||||
|
||||||
optional_title = Optional Settings | ||||||
email_title = Email Settings | ||||||
smtp_addr = SMTP Host | ||||||
|
@@ -234,6 +240,7 @@ no_reply_address = Hidden Email Domain | |||||
no_reply_address_helper = Domain name for users with a hidden email address. For example, the username 'joe' will be logged in Git as '[email protected]' if the hidden email domain is set to 'noreply.example.org'. | ||||||
password_algorithm = Password Hash Algorithm | ||||||
password_algorithm_helper = Set the password hashing algorithm. Algorithms have differing requirements and strength. `argon2` whilst having good characteristics uses a lot of memory and may be inappropriate for small systems. | ||||||
master_key_failed = Failed to generate master key: %v | ||||||
|
||||||
[home] | ||||||
uname_holder = Username or Email Address | ||||||
|
@@ -450,6 +457,7 @@ max_size_error = ` must contain at most %s characters.` | |||||
email_error = ` is not a valid email address.` | ||||||
url_error = `'%s' is not a valid URL.` | ||||||
include_error = ` must contain substring '%s'.` | ||||||
in_error = ` can contain only specific values: %s.` | ||||||
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.
Suggested change
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. Solved. |
||||||
glob_pattern_error = ` glob pattern is invalid: %s.` | ||||||
regex_pattern_error = ` regex pattern is invalid: %s.` | ||||||
unknown_error = Unknown error: | ||||||
|
@@ -2036,6 +2044,20 @@ settings.deploy_key_desc = Deploy keys have read-only pull access to the reposit | |||||
settings.is_writable = Enable Write Access | ||||||
settings.is_writable_info = Allow this deploy key to <strong>push</strong> to the repository. | ||||||
settings.no_deploy_keys = There are no deploy keys yet. | ||||||
settings.secrets = Secrets | ||||||
settings.pull_request_read = Pull Request Read | ||||||
settings.pull_request_read_info = "If allow pull request read the secret, it's security related." | ||||||
settings.pull_request_read_hint = Allow pull request read the secret | ||||||
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. 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. Solved, by removing the checkbox. |
||||||
settings.add_secret = Add Secret | ||||||
settings.add_secret_success = The secret '%s' has been added. | ||||||
settings.secret_value_content_placeholder = Input any content | ||||||
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. Isn't that placeholder redundant? Speaking of which: Do we strip surrounding whitespace already? I hope so as that is in general much more user-friendly… 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. Assuming my suggestion above is implemented:
Suggested change
Then suddenly this placeholder isn't redundant anymore. 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. Solved in the new PR. |
||||||
settings.secret_desc = Secrets could be visited by repository events | ||||||
wolfogre marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
settings.secret_content = Value | ||||||
settings.secret_key = Key | ||||||
wolfogre marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
settings.no_secret = There are no secrets yet. | ||||||
settings.secret_deletion = Remove secret | ||||||
settings.secret_deletion_desc = Removing a secret will revoke its access to this repository. Continue? | ||||||
settings.secret_deletion_success = The secret has been removed. | ||||||
settings.title = Title | ||||||
settings.deploy_key_content = Content | ||||||
settings.key_been_used = A deploy key with identical content is already in use. | ||||||
|
@@ -2355,6 +2377,7 @@ settings.update_setting_success = Organization settings have been updated. | |||||
settings.change_orgname_prompt = Note: changing the organization name also changes the organization's URL. | ||||||
settings.change_orgname_redirect_prompt = The old name will redirect until it is claimed. | ||||||
settings.update_avatar_success = The organization's avatar has been updated. | ||||||
settings.secrets = Secrets | ||||||
lunny marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
settings.delete = Delete Organization | ||||||
settings.delete_account = Delete This Organization | ||||||
settings.delete_prompt = The organization will be permanently removed. This <strong>CANNOT</strong> be undone! | ||||||
|
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.
All new files don't conform with the new headers anymore:
Sometimes, the year is still
2021
, and the other two lines should be replaced with the new header.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.
Solved.
And some files were actually created in 2021. It took too long to review.