search

UKHomeOffice / vault-sidekick

Vault sidekick
Apache License 2.0
195 stars 62 forks source link

Support custom login path for approle auth backend #92

Closed primeroz closed 4 years ago

primeroz commented 5 years ago

Allow setting a custom vault login path for approle backend

Defaults to standard /v1/auth/approle/login

Add README section with described configuration / environment variables

primeroz commented 5 years ago

Hi , this PR have been here since Aug 16 with no feedback and once again i hit the same issue and had to use a combination of "vault agent" and tokens for vault-sidekick

Is there any feedback on this PR or any other reason why it is being ignored ? Do you actually accept Contributions ? @james-bjss mentioning you since you seem to the one merging nowdays

thanks

james-bjss commented 5 years ago

Hi @primeroz I understand it's frustrating. We were were in a similar situation to yourself, but after raising this internally I was granted temp access to commit a fix for our issue.

I have raised the question about ongoing ownership of this project, so will get back to when I have a response.

With regards to your PR, it looks like a small change and seems sensible.

As this is a UK Gov project I need to check what the process is internally for accepting PRs etc.

Hope this helps to clear things up.

primeroz commented 5 years ago

@james-bjss It definetely helps thanks! I assumed you had ownership of the project but now i understand you don't , that's cool

I just think it would be useful to update the status of the project, if is dead / unmaintaned , in the README for people to look for alternatives ... I can see some important forks of this, one of them from Monzo, in case i will migrate on that and see if i can get my changes into that but i will wait and see what happens here first

thanks again for your resposne

james-bjss commented 4 years ago

Hi @primeroz sorry for taking so long to get back to you. I have now confirmed with the relevant teams in the HomeOffice that I am ok to review and merge this change. My colleague @geotechfirst has also offered to assist with the project.

I may dip into some of the other outstanding issues if I have some time in the evenings. I have agreed that we will update the Readme to reflect that there may be limited support for fixes etc.

primeroz commented 4 years ago

@james-bjss @geotechfirst Thanks a lot, this will make a huge difference to me , i have at least 3 projects running from my own branch because of this little change.

Going forward, are you guys willing to accept MR to this project ? Just to understand if is worth for me / or anyone else, spend time chasing MR on this project or maybe better just mantain my own fork and keep up to date with upstream.

I would of course prefer to get code into the upstream.

james-bjss commented 4 years ago

@primeroz - All I can say Is that I will do my best time allowing :)

I would like to get a few other fixes before cutting a new release. My only major concern is breaking exiting functionality, there doesn't seem to be anything in the way of integration tests in the project which makes me hesitant to make big changes.

primeroz commented 4 years ago

@james-bjss make sense , indeed i was thinking to start adding some testing to verify functional requirements.

Something simple, like start a vault, configure it, run vault-sidekick for feature1, cleanup and repeat for feature2 ...

I see you guys have travis already for building it so it should not be too hard to add.

james-bjss commented 4 years ago

@primeroz Merged this in for you now sorry for the delay. It made sense to take another fix alongside, but I wanted to manually verify that one. When I get some time I will take a look at some of the other issues raised.

Regarding the tests we could spin up an unsealed vault from within the test code. It's not ideal that we would need to write out actual files/clear them down between tests but would be better than nothing.