Safe logging by jeffdyke · Pull Request #47 · sirthias/scala-ssh

jeffdyke · 2019-08-08T15:17:32Z

This PR changes how commands are logged and its completely opt-in, defaulting to the normal behaviour promiscuous logging. It also adds the actual exception message to the log for easier debugging. I'm completely open to suggestions.

sirthias · 2019-08-12T11:25:10Z

.gitignore

 project/plugins/src_managed
-project/plugins/target
+project/plugins/target
+.idea


This should rather go into your dev machine's global .gitignore, not this project-level file.
(The comment at the top of this file has more details...)

sirthias · 2019-08-12T11:25:26Z

src/main/scala/com/decodified/scalassh/Command.scala

 import net.schmizz.sshj.connection.channel.direct.Session
-
-final case class Command(command: String, input: CommandInput = CommandInput.NoInput, timeout: Option[Int] = None)
+trait Command {


Let's make this a sealed trait.

Overall this was reverted, perhaps i should be a different PR.

sirthias · 2019-08-12T11:27:57Z

src/main/scala/com/decodified/scalassh/Command.scala

-
-final case class Command(command: String, input: CommandInput = CommandInput.NoInput, timeout: Option[Int] = None)
+trait Command {
+  val command: String


All those vals should be defs.
Usually abstract members are defined as defs in Scala since this leaves the decision of how to implement the member to the "implementee", without any drawbacks.

You're correct, but these are not longer here.

sirthias · 2019-08-12T11:31:53Z

src/main/scala/com/decodified/scalassh/Command.scala

+  val command: String
+  val input: CommandInput  = CommandInput.NoInput
+  val timeout: Option[Int] = None
+  val safe: Boolean


I'd say that "safe" in the context of a Command would usually be understood as sth like "safe to run", as in "doesn't do any harm".
What we are looking for here is sth more like "safe to be logged in the clear".

That was quite ambiguous, the new value that can be passed into Command is safeToLog and is still opt in.

sirthias · 2019-08-12T11:33:53Z

src/main/scala/com/decodified/scalassh/Command.scala

+                       override val timeout: Option[Int] = None,
+                       safe: Boolean = true)
+    extends Command
+case class UnsafeCommand(command: String,


The two sub-types of the ADT here are structually identical, so there appears to be little value of refactoring to an ADT in the first place.
Also, what would SafeCommand(..., safe = false) or UnsafeCommand(..., safe = true) signify?

It seems we can accomplish everything you want to do by simply adding one boolean flag to the existing Command case class, no?

This is all gone...

sirthias · 2019-08-12T11:35:43Z

src/main/scala/com/decodified/scalassh/SshClient.scala

        execWithSession(command, session)
      }
    }
+  def logCmd(command: Command): String = if (command.safe) command.command else "Sensitive data, not logged"


This appears to me as a bit coarse-grained.

What we want to achieve is not to log sensitive data, which are usually arguments to commands rather than the commands themselves.

also gone, at first i thought i would need it in more than on place, but not the case.

sirthias · 2019-08-12T11:38:28Z

Thank you, @jeffdyke, for this proposal.
I have the feeling, however, that this is not yet the best approach to solve the problem of "having a way to prevent sensitive data from being logged".

How about giving commands the ability to control their own logging?
The default could be a simple .toString (what we currently have) and sensitive commands could construct their own logging output in any way they seem fit, which would usually be a "redacted" .toString or sth like that?

jeffdyke · 2019-08-12T15:18:36Z

@sirthias thanks for your input. I'll give this some more time on your comments and clean it up. Looking at your comments i did make it a bit more verbose than i needed, so your view is appreciated.

jeffdyke

Hello @sirthias, i've gone with a much simpler approach, not sure why i was so verbose in the first place. I could open up a new PR against master if this becomes to messy.

jeffdyke · 2019-08-12T16:40:12Z

.gitignore

 project/plugins/src_managed
-project/plugins/target
+project/plugins/target
+.idea


jeffdyke · 2019-08-12T16:40:58Z

src/main/scala/com/decodified/scalassh/Command.scala

 import net.schmizz.sshj.connection.channel.direct.Session
-
-final case class Command(command: String, input: CommandInput = CommandInput.NoInput, timeout: Option[Int] = None)
+trait Command {


Overall this was reverted, perhaps i should be a different PR.

jeffdyke · 2019-08-12T16:41:16Z

src/main/scala/com/decodified/scalassh/Command.scala

-
-final case class Command(command: String, input: CommandInput = CommandInput.NoInput, timeout: Option[Int] = None)
+trait Command {
+  val command: String


You're correct, but these are not longer here.

jeffdyke · 2019-08-12T16:42:01Z

src/main/scala/com/decodified/scalassh/Command.scala

+  val command: String
+  val input: CommandInput  = CommandInput.NoInput
+  val timeout: Option[Int] = None
+  val safe: Boolean


That was quite ambiguous, the new value that can be passed into Command is safeToLog and is still opt in.

jeffdyke · 2019-08-12T16:42:15Z

src/main/scala/com/decodified/scalassh/Command.scala

+                       override val timeout: Option[Int] = None,
+                       safe: Boolean = true)
+    extends Command
+case class UnsafeCommand(command: String,


This is all gone...

jeffdyke · 2019-08-12T16:42:50Z

src/main/scala/com/decodified/scalassh/SshClient.scala

        execWithSession(command, session)
      }
    }
+  def logCmd(command: Command): String = if (command.safe) command.command else "Sensitive data, not logged"


also gone, at first i thought i would need it in more than on place, but not the case.

jeffdyke · 2019-08-13T01:06:18Z

While the comments are meaningfull, they are worthless...you'd likely rather re-review the full diff, i made it much simpler, looking forward to your thoughts, no breaking changes still. Thanks @sirthias!

jeffdyke · 2019-08-13T01:08:51Z

src/main/scala/com/decodified/scalassh/Command.scala

 object Command {
-  implicit def fromString(cmd: String): Command = Command(cmd)
+  def apply(cmd: String, safeToLog: Boolean): Command = new Command(cmd, safeToLog = safeToLog)
+  def apply(cmd: String): Command                     = new Command(cmd, safeToLog = true)


This is more for people looking at the change, it can removed, there is no need to supply safeToLog here. Obviously this can also just be added to the readme.

jeffdyke added 6 commits August 6, 2019 18:29

need to change version, but logging is safe

1af2dad

Update gitignore

7d920e4

bring back no stack trace, default to SafeCommand

9de1370

add message to Error

e3bb812

revert version

917426c

add message to Error

65481b7

sirthias reviewed Aug 12, 2019

View reviewed changes

remove the overly verbose implementation and just add a flag to command

1e7374d

jeffdyke commented Aug 12, 2019

View reviewed changes

jeffdyke commented Aug 13, 2019

View reviewed changes

Conversation

jeffdyke commented Aug 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sirthias commented Aug 12, 2019

Uh oh!

jeffdyke commented Aug 12, 2019

Uh oh!

jeffdyke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffdyke commented Aug 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants