-
Notifications
You must be signed in to change notification settings - Fork 28
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
Hide webhook password at webhook plugin xml #53
Comments
I believe the recommended practice is to use the "Generate token for a secure value" feature. If the plugin could be modified to accept such a token as a URL, I think that would be the easiest way to implement secure storage of the sensitive webhook URLs. |
Hi @Keilaron |
Hmm, #146 mentions something I wasn't aware of -- I kind of assumed that Parameters were not usable since it isn't mentioned in the UI that we can use them. |
D'oh. I'm a muppet! Yes I found the option in the actions menu. That looks super helpful. I will see how I can get at that from within plugin code. |
FYI. Making great progress on using settings from both #146 and the secure token. I have #146 working by resolving via SFeatureDescriptor items, and have extended the webhooks rest API to be able to create and edit them. So I just have the UI to write for that. For the secure tokens I have the code written and it works in unit tests, but I need to figure out how to get Spring to load a different version of the WebHookSecureResolver depending on the TeamCity version, because the underlying API in TeamCity has only been available since 2017.1 |
Raised an issue with Jetbrains about how to use Spring's factory beans. |
I have worked around the issue with spring by injecting the factory and having the factory return the relevant object. That piece of the code only contains objects that are instantiated for the duration of a webhook execution, so I don't need to rely on spring management of the instances. I need to do some more testing, but the secrets are resolving using the Preview and Test tab of the webhook dialog. |
@netwolfuk I read through #146 which brough me to this issue. I'm looking to do something similar wrt "securing" the webhook URL field. If I'm following these threads correctly, the latest alpha (currently I've installed this alpha release (both tcWebHooks & REST API + server restart) and am able to use the new webhooks-specific per-project configuration. I added a WebHook parameter to the From there I was able to configure a webhook w/ the I may have incorrectly assumed that the secure token infrastructure was being used, in which case the
For example, here's a secure value from an unrelated extension: I was hoping you could shed light on the intended behavior of creating a For context:
|
Hi @patroeper Thanks for your detailed comment. Firstly, apologies for the slow reply. My email client on my phone had stopped receiving new messages. A pile of them just arrived. WRT the secure token: yes, my intention is that the secure mechanism was invoked for parameters prefixed with There are two "secure" mechanisms in TeamCity.
From memory, it works like this...
In theory, this should create the secure value like your above screenshot I have my dev teamcity in a broken state at the moment (UI refactor) and my QA server is in pieces as I replace the fans, but will spin up a new one on 1.2alpha7 on my dev box and test this over the weekend for you. |
I don't fully understand the history of scrambled/secure token values; our TC install was ~2018ish. I found a thread that provided some context, specifically this comment: https://youtrack.jetbrains.com/issue/TW-45181#focus=Comments-27-2051540.0-0 We are using VCS for TC settings, and I observe that our secure values are written as For clarification on your notes above:
Screenshots of flow:
I think what you are saying is that
Does the latest alpha understand this layer of indirection, or is it intended to work off of the underlying scrambled token? |
Hi @patroeper. Yes the latest alphas are supposed to do the resolution of these scrambled values. What's interesting is that my TeamCity has a different format for the token. I'll need to dig further into why they're different and where that error is occurring. You could try changing the property from standard to Velocity and $secure to #secure but I suspect the issue is in the format of the token name. |
Oh. I just reread your comment. Hmm, I've never seen this credentials file before. I don't think my code will resolve that. Can you point me at some documentation on how I can setup TeamCity to use this? |
Good to know; thanks! Here's the setting on our instance (found under |
Also, one of the links I shared above has a broader discussion around the development/use of "credentials storage": https://youtrack.jetbrains.com/issue/TW-45181 |
Looking closely at your screenshot, I think I need to prepend the name, not the value with I have found that if I dig out the eg. I'll do some more experimentation, and then raise a ticket with Jetbrains for tips on how to read the credentials.json file. There must be a method they use for it, rather than me rolling my own JSON reader. |
Wow. This is exactly what you said in your first comment. I was obviously not having a good day that day. Sorry for all the confusion. |
That's correct. But even more importantly, the purpose of the
That YouTrack thread mentions a
💯
No worries. Your response times are faster than some of our paid vendors, and higher quality 🤣 |
Thanks for the thoughtful and researched replies. I now have a better understanding and a good set of ideas to try. I'm really rather hoping that setting the value correctly to secure will magically do all this for me. That's what Pavel implied. I have some time off for Easter so hopefully I'll make progress on this. |
Could you tell me what plug-in created the other parameter in your screenshot? Maybe I could see what that plug-in does to create that value. |
I believe the settings in this screenshot were created by the teamcity-oauth plugin. (based on that being the only plugin we have related to OAuth) |
@patroeper I just wanted to say a huge thank you for your persistence on this. Your keen eye has found the issue with my parameter naming, and now that I have changed it, it really just works The UX is simply...
Here is a VCS sync'd project:
And a non-VCS sync'd project:
I don't have to worry about creating tokens or referencing with the special It is still showing the URL in the history. I'll have a think about the best way to signal to the UI that the URL was built from a secure parameter. |
I have pushed to a branch which will build on teamcity.jetbrains.com. You're welcome to try a build on that branch. Hopefully I'll get a new alpha out this week which will contain this and couple of other changes. I'd like to try to get the URL shortening working for secured webhook parameters too before I release alpha 8. |
I'm interested in feedback on the way to hide the values.
|
I don't have a strong or informed opinion on this, but thinking out loud (in text)... Ignoring how much effort would be required, if I were writing a plugin, I'd want the plugin's behavior to closely match TeamCity's behavior for comparable tasks. WRT secure values, it looks like TC (sometimes) masks the values in logs with asterisks.
WRT option 3, the setting could be set on by default or have inverted meaning, but possibly at the cost of getting inbound questions from existing users losing info in the UI/logs after upgrading the plugin(s)
|
I asked the team and the reply implies that it's their logger that does the password masking. I have option 1 working. Basically I keep a flag inside the property resolver that toggles to true if any value that was secure was accessed. It does mean that I can't determine whether it was for the URL or the payload. I just know it was requested during the execution of the webhook. At which point I can choose to show or hide the entire URL or payload. However, I can't mask specific values. It's all or nothing. I'm thinking of implementing option 4 which is a combination of 1 and 3. Eg. Have a toggle to show/hide secure values and check if any were accessed. If hiding and secure, then don't log to the log. I have also implemented URL 'simplification'. So if your URL was I haven't solved the issue of it being shown in the UI because when the webhook ran it was set to 'show', but now that setting is toggled to hide in that webhook configuration. I need to think about that some more. Ideally, if the webhook has 'hide' checked now we shouldn't show URLs from previous runs. The history shown in the UI is held in memory. If someone has access to the server and manages to take a dump of memory then the full URL could be obtained from previous executions. But if they have access to the server, they could just read the logs. I can't go back in time and rewrite those either, so I'll stop worrying about that. 🤣 |
Hi @patroeper. After what can only be referred to as a terrible few months (insert reference to global pandemic here), I have a new build to test. It has the checkbox to show hide the values both in the UI and the log. Please download a build from the issue_53-hide_password branch. I have also created a page on the wiki about how to use secured values. Update: I have just noticed that toggling the checkbox does not toggle historical entries in the UI. I will take a look at that and try to get a fix asap. It should be referring to the state of that setting in the live config, but it appears to have a copy of the config in the execution stats object (that's what is written to the history store). |
A new build is available on the above link. This fixes that bug where toggling the |
@patroeper Do you want to do any testing on this, or should I just merge it? I ask because you're extremely thorough, and might pick up any issues I could have missed. |
@netwolfuk I'll take a look this week |
@netwolfuk I covered the happy-path for the
Nice job! 🎉 |
Thanks so much for the taking the time to review it @patroeper I have found one very minor unrelated bug. The code path doing a test webhook execution must be slightly different when populating the execution stats. I will endeavour to fix that bug, and then merge this branch to master and release alpha8 this weekend. I guess we'll have an alpha9 before the Release Candidate is out. I really want to ship 1.2, as it's been a couple of years now, but looking back at the last two years, I should cut myself some slack. |
We are using the webhook plugin on our corporate TeamCity.
The webhook authentication password is kept as plain text at the webhook plugin xml.
Would it be possible to store the password encrypted?
Would it be possible to mask the password at the "Extra Config" UI tab?
Thanks
Ofer
The text was updated successfully, but these errors were encountered: