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

Do not alter secret key upper-/lowercase #3375

Merged
merged 10 commits into from
Feb 20, 2024

Conversation

qwerty287
Copy link
Contributor

Don't lowercase secret names and don't uppercase env vars.

Examples:

We have a secret SECRET_UPPER (it will show up exactly like this in UI).

  • from_secret: secret_upper

    Matching with from_secret is case-insensitive, this injects SECRET_UPPER.

  • secrets: [secret_upper]

    Matching these secrets is case-insensitive. The env var however looks exactly like the string here. In this example, SECRET_UPPER is injected as secret_upper env.

  • secrets:
      - source: secret_upper
        target: some_different_name

    Here SECRET_UPPER is injected as some_different_name, it is not uppercased.

Closes #2368
Closes #3290
Closes #3096

@qwerty287 qwerty287 added bug Something isn't working server labels Feb 12, 2024
@qwerty287 qwerty287 requested a review from a team February 12, 2024 08:01
@qwerty287 qwerty287 added this to the 2.4.0 milestone Feb 12, 2024
@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Feb 12, 2024

Deployment of preview was successful: https://woodpecker-ci-woodpecker-pr-3375.surge.sh

Copy link

codecov bot commented Feb 12, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (65d88be) 36.67% compared to head (9093ae7) 36.60%.
Report is 18 commits behind head on main.

Files Patch % Lines
pipeline/frontend/yaml/compiler/convert.go 0.00% 2 Missing ⚠️
server/api/repo_secret.go 0.00% 1 Missing ⚠️
server/pipeline/stepbuilder/stepBuilder.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3375      +/-   ##
==========================================
- Coverage   36.67%   36.60%   -0.07%     
==========================================
  Files         225      225              
  Lines       14788    14822      +34     
==========================================
+ Hits         5423     5426       +3     
- Misses       8972     9003      +31     
  Partials      393      393              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@6543 6543 added the breaking will break existing installations if no manual action happens label Feb 12, 2024
@6543
Copy link
Member

6543 commented Feb 12, 2024

If some consider it a gug or not ... it was intended as feature initialy ... to have keys upercase everywhere ...

@6543 6543 changed the title Do not alter secret case Do not alter secret key value uper-/lowercase Feb 12, 2024
@6543
Copy link
Member

6543 commented Feb 12, 2024

To make it non breaking we just have to add a db migration final adjusting the values in DB and in upcomming versions including this patchset the user then can decide.

Please have a look at the docs again or are you sure you catched all places mentioning secrets?

@xoxys
Copy link
Member

xoxys commented Feb 12, 2024

If some consider it a gug or not ... it was intended as feature initialy ... to have keys upercase everywhere ...

What was the intention behind this feature? Why is it useful to have all keys uppercase? If it is useful to have them uniformed for internal use, we could just add another property to the scheme to store them unified while user-facing stuff is using the notation from the user input.

@6543
Copy link
Member

6543 commented Feb 12, 2024

no it just imitated what settings do for plugins. and try to guide the user so they do get it more often right. but if we completely get rid of the automatism, I'm fine with it too. just mind the migration ;)

@qwerty287 qwerty287 changed the title Do not alter secret key value uper-/lowercase Do not alter secret key upper-/lowercase Feb 12, 2024
@qwerty287
Copy link
Contributor Author

qwerty287 commented Feb 13, 2024

Using a migration won't make this non-breaking.

If you currently use:

secrets: [ some_secret ]

the env var is SOME_SECRET.

With this change, it will be some_secret instead because it uses exactly the value you set in the yaml, no matter whether the secret is called some_secret or SOME_SECRET.

I don't really see a way to make this non-breaking. Maybe we can hold it back for some time and do a major release at some point?

@6543
Copy link
Member

6543 commented Feb 13, 2024

Uh right :/

@lafriks
Copy link
Contributor

lafriks commented Feb 15, 2024

I'm not sure I like this change :/

@xoxys
Copy link
Member

xoxys commented Feb 15, 2024

Well manipulating user inputs without a good reason is also a bad idea and given the amount of issues regarding this a special bad thing in this particular case.

@qwerty287
Copy link
Contributor Author

@6543 To make this non-breaking, I added a fallback to inject both the name used by the user and the uppercased name for now. We can remove the uppercase one in next major.

@qwerty287 qwerty287 removed the breaking will break existing installations if no manual action happens label Feb 17, 2024
@6543
Copy link
Member

6543 commented Feb 18, 2024

If we encourage #3406 we can let it stay all uppercase or lowecase.

As you still kan have any key:

Environment:
aNYkEy:
from_secret: anykey

@6543
Copy link
Member

6543 commented Feb 18, 2024

E.g. we should make it case insensitive to users :)

@qwerty287
Copy link
Contributor Author

qwerty287 commented Feb 18, 2024

I don't think that's a good idea.

Using the secrets directive still changes the user's input which makes it harder to do debugging etc. Yes, you can easily work around this by using the from_secret syntax, but I don't think changing the user input without having a good reason (and we don't have a good reason for it) is good.

@xoxys
Copy link
Member

xoxys commented Feb 19, 2024

It's not only a debugging issue. There are situations where env vars capitalization matters. One example is Terraform/OpenTofu, see https://opentofu.org/docs/language/values/variables/#environment-variables

For example the env var TF_VAR_image_id=ami-abc123 is case-sensitive and there is currently no way to set such an env var from a secret in Woodpecker.

@lafriks
Copy link
Contributor

lafriks commented Feb 20, 2024

Why not set both uppercase and unchanged case env vars?

@xoxys
Copy link
Member

xoxys commented Feb 20, 2024

Why not set both uppercase and unchanged case env vars?

#3375 (comment)

@lafriks
Copy link
Contributor

lafriks commented Feb 20, 2024

Why not set both uppercase and unchanged case env vars?

#3375 (comment)

sorry, missed that comment. Only thing is that I don't see why we would need to deprecate it, personally I do prefer env variables upper case but not specifically like writing them in upper case in pipeline yaml 😅

@qwerty287
Copy link
Contributor Author

I'd deprecate it because it's just useless duplication.
Why don't you like writing uppercase in pipeline yamls? You would have to do it anyways in the commands.

@lafriks
Copy link
Contributor

lafriks commented Feb 20, 2024

just personal preference, not deal breaker 😉

@qwerty287
Copy link
Contributor Author

I'll merge this now, if we decide to keep both we can still remove the todo/deprecation comment in a new PR.

@qwerty287 qwerty287 merged commit 0c9bbf9 into woodpecker-ci:main Feb 20, 2024
7 of 9 checks passed
@qwerty287 qwerty287 deleted the secrets-case branch February 20, 2024 13:20
@qwerty287 qwerty287 mentioned this pull request Feb 10, 2024
@woodpecker-bot woodpecker-bot mentioned this pull request Feb 18, 2024
1 task
anbraten added a commit that referenced this pull request Mar 19, 2024
## [2.4.0](https://github.com/woodpecker-ci/woodpecker/releases/tag/2.4.0) - 2024-03-19

### 🔒 Security

- Improve security context handling
[[#3482](#3482)]
- fix(deps): update module github.com/moby/moby to v24.0.9+incompatible
[[#3323](#3323)]

### ✨ Features

- Cli setup command
[[#3384](#3384)]
- Add bitbucket datacenter (server) support
[[#2503](#2503)]
- Cli updater
[[#3382](#3382)]

### 📚 Documentation

- Delete docs for v0.15.x
[[#3508](#3508)]
- Add deployment plugin
[[#3495](#3495)]
- Bump follow-redirects and fix broken anchors
[[#3488](#3488)]
- fix: plugin doc page not found
[[#3480](#3480)]
- Documentation improvements
[[#3376](#3376)]
- fix(deps): update docs npm deps non-major
[[#3455](#3455)]
- Add "Sonatype Nexus" plugin
[[#3446](#3446)]
- Add blog post
[[#3439](#3439)]
- Add "Gradle Wrapper Validation" plugin
[[#3435](#3435)]
- Add blog post
[[#3410](#3410)]
- Extend core ideas documentation
[[#3405](#3405)]
- docs: fix contributions link
[[#3363](#3363)]
- Update/fix some docs
[[#3359](#3359)]
- chore(deps): update dependency marked to v12
[[#3325](#3325)]

### 🐛 Bug Fixes

- Fix skip setup for some general cli commands
[[#3498](#3498)]
- Move generic agent flags to cmd/agent/core
[[#3484](#3484)]
- Fix usage of WOODPECKER_DATABASE_DATASOURCE_FILE
[[#3404](#3404)]
- Set pull-request id and labels on pr-closed event
[[#3442](#3442)]
- Update org name on login
[[#3409](#3409)]
- Do not alter secret key upper-/lowercase
[[#3375](#3375)]
- fix: can't run multiple services on k8s
[[#3395](#3395)]
- Fix agent polling
[[#3378](#3378)]
- Remove empty strings from slice before parsing agent config
[[#3387](#3387)]
- Set correct link for commit
[[#3368](#3368)]
- Fix schema links
[[#3369](#3369)]
- Fix correctly handle gitlab pr closed events
[[#3362](#3362)]
- fix: update schema event_enum to remove error warning when.event
[[#3357](#3357)]
- Fix version check on next
[[#3340](#3340)]
- Ignore gitlab merge request events without code changes
[[#3338](#3338)]
- Ignore gitlab push events without commits
[[#3339](#3339)]
- Consider gitlab inherited permissions
[[#3308](#3308)]
- fix: agent panic when node is terminated during step execution
[[#3331](#3331)]

### 📈 Enhancement

- Enable golangci linter gomnd
[[#3171](#3171)]
- Apply "grpcnotrace" go build tag
[[#3448](#3448)]
- Simplify store interfaces
[[#3437](#3437)]
- Deprecate alternative names on secrets
[[#3406](#3406)]
- Store workflows/steps for blocked pipeline
[[#2757](#2757)]
- Parse email from Gitea webhook
[[#3420](#3420)]
- Replace http types on forge interface
[[#3374](#3374)]
- Prevent agent deletion when it's still running tasks
[[#3377](#3377)]
- Refactor internal services
[[#915](#915)]
- Lint for event filter and deprecate `exclude`
[[#3222](#3222)]
- Allow editing all environment variables in pipeline popups
[[#3314](#3314)]
- Parse backend options in backend
[[#3227](#3227)]
- Make agent usable for external backends
[[#3270](#3270)]
- Add no branches text
[[#3312](#3312)]
- Add loading spinner to repo list
[[#3310](#3310)]

### Misc

- Post on mastodon when releasing a new version
[[#3509](#3509)]
- chore(deps): update dependency alpine_3_18/ca-certificates to
v20240226
[[#3501](#3501)]
- fix(deps): update module github.com/google/go-github/v59 to v60
[[#3493](#3493)]
- fix(deps): update dependency @intlify/unplugin-vue-i18n to v3
[[#3492](#3492)]
- chore(deps): update dependency vue-tsc to v2
[[#3491](#3491)]
- chore(deps): update dependency eslint-config-airbnb-typescript to v18
[[#3490](#3490)]
- chore(deps): update web npm deps non-major
[[#3489](#3489)]
- fix(deps): update golang (packages)
[[#3486](#3486)]
- fix(deps): update module google.golang.org/protobuf to v1.33.0
[security]
[[#3487](#3487)]
- chore(deps): update docker.io/techknowlogick/xgo docker tag to
go-1.22.1
[[#3476](#3476)]
- chore(deps): update docker.io/golang docker tag to v1.22.1
[[#3475](#3475)]
- Update prettier version
[[#3471](#3471)]
- chore(deps): update woodpeckerci/plugin-ready-release-go docker tag to
v1.1.0 [[#3464](#3464)]
- chore(deps): lock file maintenance
[[#3465](#3465)]
- chore(deps): update postgres docker tag to v16.2
[[#3461](#3461)]
- chore(deps): update lycheeverse/lychee docker tag to v0.14.3
[[#3429](#3429)]
- fix(deps): update golang (packages)
[[#3430](#3430)]
- More `when` filters
[[#3407](#3407)]
- Apply `documentation`/`ui` label to corresponding renovate updates
[[#3400](#3400)]
- chore(deps): update dependency eslint-plugin-simple-import-sort to v12
[[#3396](#3396)]
- chore(deps): update typescript-eslint monorepo to v7 (major)
[[#3397](#3397)]
- fix(deps): update module github.com/google/go-github/v58 to v59
[[#3398](#3398)]
- chore(deps): update docker.io/techknowlogick/xgo docker tag to
go-1.22.0
[[#3392](#3392)]
- chore(deps): update docker.io/golang docker tag
[[#3391](#3391)]
- fix(deps): update golang (packages)
[[#3393](#3393)]
- chore(deps): update docker.io/woodpeckerci/plugin-docker-buildx docker
tag to v3.1.0
[[#3394](#3394)]
- Add link checking
[[#3371](#3371)]
- Apply `dependencies` label to all PRs
[[#3358](#3358)]
- chore(deps): update docker.io/woodpeckerci/plugin-docker-buildx docker
tag to v3.0.1
[[#3324](#3324)]

---------

Co-authored-by: 6543 <[email protected]>
Co-authored-by: Anbraten <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server
Projects
None yet
5 participants