Safe logging #47
Safe logging #47
Conversation
.gitignore
Outdated
| project/plugins/target | ||
| .idea |
There was a problem hiding this comment.
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...)
| @@ -18,11 +18,26 @@ package com.decodified.scalassh | |||
|
|
|||
| import java.io.{ByteArrayInputStream, File, FileInputStream, InputStream} | |||
| import net.schmizz.sshj.connection.channel.direct.Session | |||
|
|
|||
| final case class Command(command: String, input: CommandInput = CommandInput.NoInput, timeout: Option[Int] = None) | |||
| trait Command { | |||
There was a problem hiding this comment.
Let's make this a sealed trait.
There was a problem hiding this comment.
Overall this was reverted, perhaps i should be a different PR.
|
|
||
| final case class Command(command: String, input: CommandInput = CommandInput.NoInput, timeout: Option[Int] = None) | ||
| trait Command { | ||
| val command: String |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You're correct, but these are not longer here.
| val command: String | ||
| val input: CommandInput = CommandInput.NoInput | ||
| val timeout: Option[Int] = None | ||
| val safe: Boolean |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
That was quite ambiguous, the new value that can be passed into Command is safeToLog and is still opt in.
| override val timeout: Option[Int] = None, | ||
| safe: Boolean = true) | ||
| extends Command | ||
| case class UnsafeCommand(command: String, |
There was a problem hiding this comment.
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?
| @@ -53,9 +52,10 @@ final class SshClient(val config: HostConfig) extends ScpTransferable { | |||
| execWithSession(command, session) | |||
| } | |||
| } | |||
| def logCmd(command: Command): String = if (command.safe) command.command else "Sensitive data, not logged" | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
also gone, at first i thought i would need it in more than on place, but not the case.
|
Thank you, @jeffdyke, for this proposal. How about giving commands the ability to control their own logging? |
|
@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. |
.gitignore
Outdated
| project/plugins/target | ||
| .idea |
| @@ -18,11 +18,26 @@ package com.decodified.scalassh | |||
|
|
|||
| import java.io.{ByteArrayInputStream, File, FileInputStream, InputStream} | |||
| import net.schmizz.sshj.connection.channel.direct.Session | |||
|
|
|||
| final case class Command(command: String, input: CommandInput = CommandInput.NoInput, timeout: Option[Int] = None) | |||
| trait Command { | |||
There was a problem hiding this comment.
Overall this was reverted, perhaps i should be a different PR.
|
|
||
| final case class Command(command: String, input: CommandInput = CommandInput.NoInput, timeout: Option[Int] = None) | ||
| trait Command { | ||
| val command: String |
There was a problem hiding this comment.
You're correct, but these are not longer here.
| val command: String | ||
| val input: CommandInput = CommandInput.NoInput | ||
| val timeout: Option[Int] = None | ||
| val safe: Boolean |
There was a problem hiding this comment.
That was quite ambiguous, the new value that can be passed into Command is safeToLog and is still opt in.
| override val timeout: Option[Int] = None, | ||
| safe: Boolean = true) | ||
| extends Command | ||
| case class UnsafeCommand(command: String, |
| @@ -53,9 +52,10 @@ final class SshClient(val config: HostConfig) extends ScpTransferable { | |||
| execWithSession(command, session) | |||
| } | |||
| } | |||
| def logCmd(command: Command): String = if (command.safe) command.command else "Sensitive data, not logged" | |||
There was a problem hiding this comment.
also gone, at first i thought i would need it in more than on place, but not the case.
|
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! |
|
|
||
| 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) |
There was a problem hiding this comment.
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.
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.