VAULT-12798 Correct removal behaviour when JWT is symlink #18863

VioletHynes · 2023-01-26T19:17:36Z

The current behaviour was:

Agent would correctly read the JWT by following the symlink
Agent would then delete the symlink, instead of the JWT (when remove_jwt_after_reading is set to true)

In other words, it followed the symlink correctly for reading, but incorrectly for deleting.

I've added a new option, remove_jwt_follows_symlinks, that will change this behaviour when set to delete the JWT, instead of the symlink, when removing the JWT.

VioletHynes · 2023-01-26T19:19:10Z

command/agent/testing.go

@@ -61,7 +60,7 @@ func GetTestJWT(t *testing.T) (string, *ecdsa.PrivateKey) {
 }

 func readToken(fileName string) (*logical.HTTPWrapInfo, error) {
-	b, err := ioutil.ReadFile(fileName)
+	b, err := os.ReadFile(fileName)


ioutil.ReadFile is deprecated.

command/agent/auth/jwt/jwt.go

peteski22 · 2023-01-27T15:47:04Z

command/agent/jwt_end_to_end_test.go

@@ -83,16 +104,24 @@ func testJWTEndToEnd(t *testing.T, ahWrapping bool) {

 	// We close these right away because we're just basically testing
 	// permissions and finding a usable file name
-	inf, err := ioutil.TempFile("", "auth.jwt.test.")
+	inf, err := os.CreateTemp("", "auth.jwt.test.")


❓ If we used t.TempDir() in the paths (dir), would that allow us to drop the close/cleanup lines?

It wouldn't - we'd still need to check if the file exists etc and clean it up once we're done with it

command/agent/auth/jwt/jwt.go

…tion

digital-content-events · 2023-03-08T20:13:05Z

📄 Content Checks

Updated: Wed, 08 Mar 2023 20:12:52 GMT

Found 2 error(s)

`content/docs/agent/autoauth/methods/jwt.mdx`

Position	Description	Rule
`9:65-10:24`	Unexpected product-relative link: `/docs/auth/jwt`. Ensure that relative links are fully-qualified Developer paths: `/{productSlug}/docs/auth/jwt`	`ensure-valid-link-format`
`30:6-30:63`	Unexpected product-relative link: `/docs/concepts/duration-format`. Ensure that relative links are fully-qualified Developer paths: `/{productSlug}/docs/concepts/duration-format`	`ensure-valid-link-format`

digital-content-events · 2023-03-08T20:13:05Z

📄 Content Checks

Updated: Wed, 08 Mar 2023 20:12:52 GMT

Found 2 error(s)

`content/docs/agent/autoauth/methods/jwt.mdx`

Position	Description	Rule
`9:65-10:24`	Unexpected product-relative link: `/docs/auth/jwt`. Ensure that relative links are fully-qualified Developer paths: `/{productSlug}/docs/auth/jwt`	`ensure-valid-link-format`
`30:6-30:63`	Unexpected product-relative link: `/docs/concepts/duration-format`. Ensure that relative links are fully-qualified Developer paths: `/{productSlug}/docs/concepts/duration-format`	`ensure-valid-link-format`

VioletHynes · 2023-03-08T20:15:38Z

Updated following discussion on Slack: now requires remove_jwt_follows_symlinks opt-in to follow the symlink to delete the JWT, otherwise it deletes the symlink.

Sorry this took a while to update, I had lots of other priorities and this one didn't seem like a good plate to spin.

ncabatoff · 2023-03-14T19:12:29Z

website/content/docs/agent/autoauth/methods/jwt.mdx

@@ -18,3 +18,13 @@ method](/docs/auth/jwt).
 - `remove_jwt_after_reading` `(bool: optional, defaults to true)` -
  This can be set to `false` to disable the default behavior of removing the
  JWT after it's been read.
+
+- `remove_jwt_follows_symlinks` `(bool: optional, defaults to false)` -


I think this is ok. That said, my intuition expected that with this and remove_jwt_after_reading set to true, both the symlink (and any intervening symlinks) as well as the ultimate JWT file referenced would be deleted. An alternative to consider: leave remote_jwt_after_reading behaviour unchanged from prior to PR, i.e. it always only affects the filename referenced in the config, be it file or symlink, and make the new config option control whether or not the symlink target gets deleted.

How about an alternative approach:
We rename remove_jwt_follows_symlinks to remove_jwt_behavior, a string with options symlink and jwt - it hopefully disambiguates the behaviour, as well as keeps the door open to support a both, in the future?

I guess this could also be over-engineering the solution, too, as realistically I think the majority of customers won't want both (and the majority, in my intuition, will only want the jwt behaviour).

Yeah, maybe we should just stick with what you've got, we've already spent a long time on what I believe to be a niche feature.

Yeah - I agree that this is fairly niche. Thanks for the review, though, and thanks for sticking through with this!

* VAULT-12798 testing for jwt symlinks * VAULT-12798 Add testing of jwt removal * VAULT-12798 Update docs for clarity * VAULT-12798 Small change, and changelog * VAULT-12798 Lstat -> Stat * VAULT-12798 remove forgotten comment * VAULT-12798 small refactor, add new config item * VAULT-12798 Require opt-in config for following symlinks for JWT deletion * VAULT-12798 change changelog

VioletHynes added 3 commits January 24, 2023 15:34

VAULT-12798 testing for jwt symlinks

71419b9

VAULT-12798 Add testing of jwt removal

eeb856f

VAULT-12798 Update docs for clarity

c0ec6d4

VioletHynes commented Jan 26, 2023

View reviewed changes

command/agent/auth/jwt/jwt.go Outdated Show resolved Hide resolved

vercel bot deployed to Preview January 26, 2023 19:21 View deployment

VAULT-12798 Small change, and changelog

40dbfb1

VioletHynes added this to the 1.13.0-rc1 milestone Jan 26, 2023

VioletHynes requested review from kubawi, peteski22 and ncabatoff January 26, 2023 19:48

VioletHynes marked this pull request as ready for review January 26, 2023 19:49

VioletHynes requested a review from yhyakuna as a code owner January 26, 2023 19:49

ncabatoff reviewed Jan 27, 2023

View reviewed changes

command/agent/auth/jwt/jwt.go Outdated Show resolved Hide resolved

VioletHynes added 2 commits January 27, 2023 09:34

VAULT-12798 Lstat -> Stat

0424c98

VAULT-12798 remove forgotten comment

7bec2e3

peteski22 reviewed Jan 30, 2023

View reviewed changes

VAULT-12798 small refactor, add new config item

81646e3

vercel bot deployed to Preview January 30, 2023 14:25 View deployment

peteski22 approved these changes Jan 30, 2023

View reviewed changes

averche modified the milestones: 1.13.0-rc1, 1.14 Feb 13, 2023

VioletHynes added 2 commits March 8, 2023 15:11

VAULT-12798 Require opt-in config for following symlinks for JWT dele…

06b8bd7

…tion

VAULT-12798 change changelog

ff3269b

VioletHynes requested a review from ncabatoff March 8, 2023 20:15

ncabatoff reviewed Mar 14, 2023

View reviewed changes

ncabatoff approved these changes Mar 14, 2023

View reviewed changes

VioletHynes merged commit 5581c26 into main Mar 14, 2023

VioletHynes deleted the violethynes/VAULT-12798 branch March 14, 2023 19:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VAULT-12798 Correct removal behaviour when JWT is symlink #18863

VAULT-12798 Correct removal behaviour when JWT is symlink #18863

VioletHynes commented Jan 26, 2023 •

edited

Loading

VioletHynes Jan 26, 2023

peteski22 Jan 27, 2023

VioletHynes Jan 30, 2023

digital-content-events bot commented Mar 8, 2023

`content/docs/agent/autoauth/methods/jwt.mdx`

digital-content-events bot commented Mar 8, 2023

`content/docs/agent/autoauth/methods/jwt.mdx`

VioletHynes commented Mar 8, 2023

ncabatoff Mar 14, 2023

VioletHynes Mar 14, 2023

VioletHynes Mar 14, 2023

ncabatoff Mar 14, 2023

VioletHynes Mar 14, 2023

VAULT-12798 Correct removal behaviour when JWT is symlink #18863

VAULT-12798 Correct removal behaviour when JWT is symlink #18863

Conversation

VioletHynes commented Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

digital-content-events bot commented Mar 8, 2023

📄 Content Checks

content/docs/agent/autoauth/methods/jwt.mdx

digital-content-events bot commented Mar 8, 2023

📄 Content Checks

content/docs/agent/autoauth/methods/jwt.mdx

VioletHynes commented Mar 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VioletHynes commented Jan 26, 2023 •

edited

Loading

`content/docs/agent/autoauth/methods/jwt.mdx`

`content/docs/agent/autoauth/methods/jwt.mdx`