ssh: disable repository config password

[ghost]

• It would prevent people from pushing their passwords to public Git hosting servers
• I don't see any use case where this is "needed" or at least "desired"

Use --local, --system or --global

[ghost]

@efiop, why this is not important? 🤔

This is a design flaw that can expose user's credentials

[efiop]

@MrOutis Not everyone is really comfortable with ssh keys and might just want to try it out first with a password, same as he is able to simply ssh into another box using a password. So it doesn't harm to leave it as an option, there are a lot of other ways one could shoot himself in the leg, and this one is a pretty obvious one and easy to avoid.

[ghost]

my bad, @efiop , I didn't want to disable the global config 🙈 , but the repository config; if you are using "password" it shouldn't end in the .dvc/config file

[efiop]

@MrOutis But you might want to have a shared password, who knows :) I mean, the care should be taken, but is this really something that we should forbid specifically for .dvc/config? Seems excessive.

[shcheklein]

It would be nice to have a warning at least? It can save a lot of time. It's one of the common mistakes people do - commit credentials to github.

[efiop]

@shcheklein That also seems like an unnecessary complication. Generally not putting passwords into your configs is a piece of common knowledge, I don't think we need to explicitly protect against that. The implementation is not trivial and the value is questionable, we haven't seen any complaints about it at all. Also, if someone is indeed doing that for a reason, then he will be not happy about warnings and will ask us to implement a config option to disable that specific warning (like guys using fish), which is yet another piece of code that we don't need with questionable usefulness.

[shcheklein]

@efiop It's not a "common knowledge". Even if people are aware about this problem they still keep doing it for a variety of reasons. There are even tools like this https://github.com/awslabs/git-secrets to prevent this from happening. There are blog posts like this: https://help.github.com/en/articles/removing-sensitive-data-from-a-repository and like this https://pomeroy.me/2017/10/stop-committing-credentials-to-github/ (with some links to how easy it is to discover secrets on public Github, and on private - don't even ask me - it's terrible :) ).

May be I'm missing something, but why is it "non-trivial"? I also don't see how a simple message (I would actually prefer aborting in this case, and when you try to save S3/GCS/etc credentials to project's config) can harm the interface.

I understand that there is a philosophy of building tools for "professionals" - brutal command line, no docs, you know what you do philosophy. Following it we should be overwriting data for example - because, users should be aware about what they doing. But, even if I share it in general, I don't see any reasons of not putting some nice safe-guards, nice help messages that can save someone a lot of time. After all trying to save password to the project's config is a mistake in 99% of the time. May be even 100%. (at least I would never allow this in my team).

Also, another consideration. Even if I'm aware about the concept of not committing sensitive stuff, it's not immediately clear that dvc remote modify leads to this. People can run git commit -a -m (not a good idea by itself, but we are humans) after that easily.

[ghost]

@MrOutis But you might want to have a shared password, who knows :) I mean, the care should be taken, but is this really something that we should forbid specifically for .dvc/config? Seems excessive.

Agree, @efiop , I have no problem closing this one

[efiop]

@shcheklein Sorry for not replying to each part of your comment, but I will ask another question instead to save us some time. Ok, so storing your password in .dvc/config is bad because it will be stored in git. Isn't generally storing passwords in plain text harmful? Haven't we seen a lot of fuss about that? Yeah, we've seen that. So now we need to protect users from using password in config at all, right? Warnings? Or should we simply remove it from config at all? I would remove that, but people might want to use it for multiple reasons.

[shcheklein]

@efiop I don't suggest removing it. Just make this action a bit more explicit. Implementation seems to me simple, and it can vary from a simple warning (that what we already do in our docs, btw) to only allow changing it manually, for example. You will have to just put a bit of extra effort (in case of warning even no extra effort) to put secrets into .dvc/config.

(please, let's not shift the discussion to a more general topic - plain text secrets, we can discuss it separately to keep this conversation focused)

Btw, what are the reasons to save secrets into Git? Can you name me or point me to some examples?

[ghost]

Btw, what are the reasons to save secrets into Git? Can you name me or point me to some examples?

@shcheklein , some people like to have credentials in the repository (encrypt & push -> pull & decrypt) https://guides.rubyonrails.org/security.html#custom-credentials

[efiop]

@shcheklein So you suggest only printing a warning on dvc config / remote modify, so we don't have to have a config option to disable that. Ok, but if we implement that, the question would be "why are we not checking that at the runtime when loading dvc config?" to protect against setting those things by hand or to notify about this next time after dvc config/remote modify, just in case user has missed it. Doing what you propose is a half-measure, and would be considered as a bug later.

I was not trying to shift the discussion with the previous question about storing your passwords in plain text. I was simply trying to illustrate that we should either do all or nothing in this case, or at least consider that. If we decide to protect against such an obvious thing as storing your password in git, why not then protect against other common things such as plain-text passwords?

@MrOutis @shcheklein Since the question got raised and supported by both of you guys, let's indeed somehow protect against this issue. Is printing a warning on dvc config/remote modify(but not during the runtime when parsing the config in other commands) something that everyone agrees on? If so, let's do that. How about p3-nice-to-have as a priority?

[shcheklein]

@efiop my thought process was - if you decided to manually edit the file or ignored the warning or ignored the prompt (depending on the implementation) - then you definitely understand what are you doing and you actually want to save them in Git for whatever reason.

why not then protect against other common things such as plain-text passwords?

Are you talking about global/system/local configs? It's just less obvious for me and I would need to think a bit more - for a variety of reasons. It's way easier to fix that mistake (you don't push it somewhere). There are less security risks - you have to breach the machine to read the config. Etc.

[ghost]

@efiop , I already agreed on leaving this as it is: #1615 (comment)

If we start thinking about all the scenarios where we can protect the user, we will end up with a lot of warnings in DVC (or at least that's my impression)

[efiop]

@MrOutis You've raised the initial question, so I felt like you've agreed under the pressure :) Ok, in that case, we don't have a consensus with @shcheklein . In general, it feels like we are spending too much time on such a minor thingy.

[ghost]

@efiop , don't get me wrong 😅
I took a more relaxed approach with warnings, in general, after reading your comments.

there are a lot of other ways one could shoot himself in the leg, and this one is a pretty obvious one and easy to avoid.

I see it more as a design question, sort of related to this: #2498

Should we warn the user against every possible pitfall? Should we put boundaries to prevent the user from going that direction? Should we trust them (delegating responsibilities to them)?

[shcheklein]

agreed that this discussion are related. If take a look at git or heroku CLI they do warn or abort/prompt on certain things or help you with nice things like when you make a typo in a command. This is what good UI/UX for - to support you (by not breaking the workflow of course).

In this particular case it's just feels that probability of you running this by mistake (not realizing that this is bad idea, or that there is --local/--global option, etc) is SO high and price of a mistake is high as well that we might want to put an extra barrier.

[efiop]

@shcheklein

agreed that this discussion are related. If take a look at git or heroku CLI they do warn or abort/prompt on certain things or help you with nice things like when you make a typo in a command. This is what good UI/UX for - to support you (by not breaking the workflow of course).

I agree that those are good examples of a shiny friendly CLI with bells and whistles. And I like those and we've been adding more friendly things for a long time. But this specific example is so minor and so insignificant with 0 real complains, that I don't think we should spend so much time on it.

but I don't think it is worth spending our focus, so that is why I was suggesting closing this issue, as even if we keep it open, we won't ever come to implementing this, unless someone complains, at which point the priority would be instantly bumped, as there would be clear proof that fixing this will help someone.

if the probability is "SO high" along with a "high price", how come we haven't ever heard of anyone complaining about this? We did hear people complaining about seemingly much less frequent and less dangerous things than this one, but never about this? Seems strange. We could place a half-measure warning that you suggest, but I don't think it is worth more than p4 or p3 max, which means that we will probably never come to implementing this, unless some user comes and complains about this, at which point we will instantly bump the priority, as it will make it clear that it will help someone.

[shcheklein]

so minor and so insignificant

Why don't we then just disable this (to be safe) and wait for people to ask to enable it back? If it's so minor and "insignificant" - there is nothing to worry about at all, no one will complain, or will show them how to use it properly. This is the easiest and proper way to understand if there are use cases for storing password in Git.

We did hear people complaining about seemingly much less frequent

I doubt that anyone will ever complain about it (though we had one like for this issue, right?) - it's not a bug, it's an UI/workflow improvement. Also, people do this and they don't realize the consequences.

if the probability is "SO high" along with a "high price" ...

probability is high that you are doing a mistake if you are doing it, not that you are going to do it - those are two different things.

We could place a half-measure warning that you sugges..

That's why I don't like it tbh. I would put a higher barrier - only manual/explicit updates to the config at least. And would wait some complaint from the community.

means that we will probably never come to implementing this

p4 or p3 max and never implementing them - it can be a good first issue for contributors or hacktroberfest or for GSOC, etc, or someone will come and upvote it indeed.

[dashohoxha]

If it is not too much work, I think this would be a useful feature, i.e. preventing users to add a password (or some other private credentials), but allowing them to add it manually anyway (by editing the config file).

However I agree that this is neither urgent, not high priority. If making a first stable release would be a milestone, then I would postpone it beyond that milestone.

In any case, it is not a bad idea to have a list of nice-to-have features, even if they are never implemented.

[efiop]

Why don't we then just disable this (to be safe) and wait for people to ask to enable it back? If it's so minor and "insignificant" - there is nothing to worry about at all, no one will complain, or will show them how to use it properly. This is the easiest and proper way to understand if there are use cases for storing password in Git.

I'm pretty sure that we've agreed on this already a few comments above.

I doubt that anyone will ever complain about it (though we had one like for this issue, right?) - it's not a bug, it's an UI/workflow improvement. Also, people do this and they don't realize the consequences.

Ok, so it is a minor improvement that is not really important. Let's leave it as is.

probability is high that you are doing a mistake if you are doing it, not that you are going to do it - those are two different things.

Indeed, missed this one. Thanks.

That's why I don't like it tbh. I would put a higher barrier - only manual/explicit updates to the config at least. And would wait some complaint from the community.

👍

p4 or p3 max and never implementing them - it can be a good first issue for contributors or hacktroberfest or for GSOC, etc, or someone will come and upvote it indeed.

Ok, let's leave it as is.

[efiop]

Closing as stale.