Revert "Fix editing connection with sensitive extra field" #53888
Conversation
This reverts commit 8ef792d.
pierrejeambrun
requested review from
bbovenzi,
bugraoz93,
ephraimbuddy,
jason810496,
jscheffl,
rawwar,
ryanahamilton and
shubhamraj-git
as code owners
boring-cyborg
bot
added
area:API
|
@pierrejeambrun I think it is not good to "just revert" the PR which had been discussed and agreed. It was merged by this by intend and proper review. If you think we should change it we should re-discuss and then change, not just revert. |
|
@jscheffl This was reverted as is a security concern -- this ends up exposing passwords to the UI -- #53753. It's also just generally a bad idea to send passwords back in responses when they aren't needed (which they aren't here, as the only reason they were sent was so that connections can be edited in place, but as mentioned today in the original PR, there are other ways of achieving that), and the only thing that should need passwords in the plain is executing tasks. From an audit point of view, having the passwords exposed in the API to the UI means that any time a user leaves a company you should really be rotating every single connection they might have ever viewed. |
|
Let's discuss it in mailing thread. I think @ashb you do not realise that it has always been like that and while I agree that having "write-only" sensitive information is generally "safer" than sending passwords back-and-forth - this is a completely new feature that had never been a goal of connection editing API (not for last 5 years at least) and we accepted it as part of our Security Model and we discussed it in the past that this is "accepted" thing. And this PR does not change it. It basically changes masking behaviour. |
|
Showing passwords makes us look like clowns, I'm amazed you think that is okay behaviour for a webapp that cares at all about security @potiuk. |
) * Revert "Fix editing connection with sensitive extra field (apache#52403)" This reverts commit 8ef792d. * Fix CI
This means that we were clowns for a long time and we never raised the issue despite it being discussed several times. It's a strange thing to see it raised now and not then. I think it was a choice made long time ago (not by me) and I would be rather careful with pejorative words like "clowns". It's a choice. We all agreed to it. We agreed to the model. We discussed it several time in the past and nobody ever raised concern for it (including you who insisted to be on all security discussions). Yes. I agree that "write only" is better for security - and I even proposed it in the past discussions for it. I was not even a proponent of returning passwords - if you look at past discussions, I always told "write-only" is a better solution. I merely explained what choice we made (all of us - including you if you did not oppose those choices, where you were part of those discussions in security team and had a chance to state the "clown" argument before). Many apps (for example password managers - including Google Password manager for example) make the choice that they expose password to clients (stored for manipulation and editing in a CRUD form). Is that web app "clown" app? I am not sure I would use that word. Security is never 0/1 and "obvious". Security is a set of choices to make. And so far we (yes, including you - all the important decisions we made here are "us") made a choice that we include passwords in API responses (under certain condidtions - EDIT access to connections). We are now changing that choice - and I am super happy about it - but I would never say "clowns" - for choice you also were part of. I find it surprsing to use such pejorative statement in professional context. |
) * Revert "Fix editing connection with sensitive extra field (apache#52403)" This reverts commit 8ef792d. * Fix CI
) * Revert "Fix editing connection with sensitive extra field (apache#52403)" This reverts commit 8ef792d. * Fix CI
4 participants
Reverts #52403