feat: added skip-hash for basic auth for Konnect#1765
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1765 +/- ##
==========================================
- Coverage 32.79% 32.71% -0.08%
==========================================
Files 73 73
Lines 8093 8112 +19
==========================================
Hits 2654 2654
- Misses 5273 5292 +19
Partials 166 166 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| "thus gaining some performance with large configs.\n"+ | ||
| "Usage of this flag without apt select-tags and default-lookup-tags can be problematic.\n"+ | ||
| "This flag is not valid with Konnect.") | ||
| syncCmd.Flags().BoolVar(&dumpConfig.SkipHashForBasicAuth, "skip-hash-for-basic-auth", |
There was a problem hiding this comment.
Do we not want this flag for gateway apply?
There was a problem hiding this comment.
Also, in cases where the basic auth has been synced before without the flag, using this flag leads to no diff and so no action - that is expected, right?
There was a problem hiding this comment.
Yeah. That is expected.
The GET /basic-auth endpoint doesn't have any skip-hash param. So, there's no way of knowing if the returned password is a hash or not. Earlier too we didn't compare for passwords while syncing/diffing. So, I am retaining the same behaviour.
Prashansa-K
force-pushed
the
feat/basic-auth-skip-hash
branch
from
067bc53 to
15b64a5
Compare
Prashansa-K
force-pushed
the
feat/basic-auth-skip-hash
branch
from
4baf58a to
c14ff4f
Compare
| return true | ||
| } | ||
|
|
||
| if targetContent.Info != nil && targetContent.Info.SkipHashForBasicAuth { |
There was a problem hiding this comment.
Why is this block necessary? 🤔
There was a problem hiding this comment.
A user can choose to pass a command-line flag or use the info block to trigger the skip-hash functionality. So, this block looks at the info block to see if there's a skip-hash there.
There was a problem hiding this comment.
Not blocking, for knowledge purposes - we don't write the bool to the Info field on dump though, correct? Is adding field to Info block a common approach users take?
There was a problem hiding this comment.
we don't write the bool to the Info field on dump though, correct?
For this one, we can't. This is because the GET /basic-auth endpoint doesn't tell us if the password is a hash or not.
So, this particular thing is not valid for dump.
Is adding field to Info block a common approach users take?
Yes. The metadata remains a part of the config. So, it is "safer", as folks can forget to pass the CLI flag.
3 participants
This feature introduces a flag to be used with sync
command to skip-hashing of basic-auth credential
passwords when using with Konnect.
An info field is also added for the same.
Linked PRs:
Kong/go-kong#576
Kong/go-database-reconciler#342
For #1747