fix: add warning for long secret names #359
No reviewers
decentral1se
Labels
No Label
bug
build
ci/cd
contributing
design
documentation
duplicate
enhancement
help wanted
invalid
meta
question
security
wontfix
No Milestone
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: coop-cloud/abra#359
Loading…
Reference in New Issue
No description provided.
Delete Branch "rix/abra:add-secret-length-linting"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
A start of a fix for coop-cloud/organising#463
Putting some code out to start a discussion. I've added a linting rule for recipes to establish a general principal but I want to put some validation into cli/app/new.go as that's the point we have both the recipe and the domain and can say for sure whether or not the secret names lengths cause a problem but that will have to wait for a bit. Let me know if I've missed the mark somewhere
ac5f59cf37toa150f227bcLooking good!
Yeh, doing some
app new ...scary alerta alerta warning might be ideal and help avoid frustration further down the line.If you're interested in writing a test, I'd say adding something to https://git.coopcloud.tech/coop-cloud/abra/src/branch/main/tests/integration/recipe_lint.bats might be nice. Simply adding a
compose.secret-too-long.ymlto https://git.coopcloud.tech/coop-cloud/abra-integration-test-recipe would allow you to reproduce the warning (i.e. bysed'ingCOMPOSE_FILE="compose.yml:compose.secret-too-long.yml"into the.env).@ -118,0 +119,4 @@Ref: "R014",Level: "warn",Description: "Long secret names",HowToResolve: "Reduce the lenght of secret names to 12 characters.",%s/lenght/length/Good spot! fixed now : )
a150f227bcto82bcaaa86fJust thought I'd update on this. As is turning into a theme with this one, it was harder than I thought! I wrote my code to catch a failing test case and then realised it doesn't actually work because it's already failed before it gets to where I put my method 🤦♂️
I'll re-work it to actually work at some point and I promise to come back and writes tests for it at some point : )
666e9a041ctod78abfec2fSome more progress, the error now shows up before the whole thing falls over which is an improvement! I've created the data for an integration test but not the intergation test itself as of yet.
One question I did have is that at the moment the error will only show up if you specify the -S flag on the new command to generate secrets. In some ways this seems reasonable as, 'no secrets, no problem' is at least ostensibly true but I'm imaging you'll need to generate those secrets on deploy anyway.
Do we want the same check on the deploy as well? It would seem to be sensible as secret generation on 'abra new' isn't enabled by default.
Let me know your thoughts and apologies that this PR seems to be like Zeno's arrow : )
No worries @rix! Doing great work here. If you don't
--secretsonabra app newthen when you deploy, it will fail with missing secrets. Not very gracefully actually, but I'll open a ticket for that. Then you'd probably runabra app secret generate <domain> -a. Maybe a call toensureSecretLengthsis also worth it there before generation?Another pass 🏃♀️
@ -258,1 +263,4 @@}func ensureSecretLengths(secrets map[string]string, domainName string, sanitisedAppName string) error {if len(sanitisedAppName) > 45 {Maybe it's time to abstract this
45to aconstinpkg/config/env.go? We're starting to use it in several places and this will probably lead to confusion at best and bugs at worst as things change over time.Done : )
@ -259,0 +272,4 @@maxSecretLength := 0for secretName := range secrets {nitpick: drop extra newline?
Think I've got this one now but let me know if I misinterpreted which line you were talking about : )
@ -259,0 +277,4 @@maxSecretLength = len(secretName)}if len(secretName)+domainAndFormatLength > 64 {Maybe
64can also go inpkg/config/env.gowhile we're at it?Done
@ -259,0 +278,4 @@}if len(secretName)+domainAndFormatLength > 64 {failingSecrets = append(failingSecrets, secretName)I think it would be simpler to just error out here. Also a simpler/clearer error message to write?
I've made it drop out after the first error now but I guess the side effect is that if there's more than one long secret it will be a bit of a whack-a-mole to find them all but I guess it's a pretty rare occurence so the trade off for cleaner code is probably worth it : )
@ -259,0 +287,4 @@return fmt.Errorf("the following secrets are too long to work with the domain name %s\n change their length or use a shorter domain name:\n %s", domainName, failedSecretsString)}logrus.Debugf(Unsure if this debug logging is necessary? It would also simplify the logic above and we could drop
maxSecretLengthand the accompanyingifstatement?Removed, it helped me with debugging and I thought that maybe it might help explain the logic but you're right that's it's probably not helpful for the day to day experience.
Thanks @decentral1se : ) I think the change for abra app secret generate and the integration test (I haven't checked the status of the test repo PR yet) are the last remaining things after this : )
Hi @rix,
there are some merge conflicts, which are propably caused by my pr #394 (sorry about that). Could you try and resolve these? I can help of course or do it for you if you want :)
@rix shall we try to get this merged? Unsure of status atm but down to help ✊
Sorry or the delay in getting back to you, I did reply to my e-mail notification thinking that the comment would show up but it seemingly hasn't. I've been a bit burned out for a while but I'm trying to get back to looking at this now. I've done a basic merge from main on the branch but it looks like there's been a number of changes (for the better!) in the code that I will need to build into this. It looks like the new secret struct is going to make this easier than it was going to be initially but it will take me a little while to get my head around : ) I think I should be able to take a longer look tomorrow so I'll let you know how that goes : )
Hey, I've just pushed up some new stuff that I think works with the flow moving the verification as part of the read secret config since we seem to have all the data we need now. I've also added a test to the secret stuff that hopefully should illustrate what I'm going for. I feel like there's some scenario I'm potentially missing though so would appreciate some feedback. I'd be delighted to pair on this if you want to get into it but due to work it'd either have to a lunch time (somewhere between 12 - 2pm) or an evening.
Either way, let me know what you think about the new code : )
WIP: Add warning for long secret names.to fix: add warning for long secret namestysm @rix for coming back to this, let's merge it! It's looking solid.
Hope you're doing OK 💝