Skip to content

Safe logging #47

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.

Sign up for GitHub

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

Open
jeffdyke wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Safe logging #47

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1af2dad
need to change version, but logging is safe
jeffdyke Aug 6, 2019
7d920e4
Update gitignore
jeffdyke Aug 6, 2019
9de1370
bring back no stack trace, default to SafeCommand
jeffdyke Aug 8, 2019
e3bb812
add message to Error
jeffdyke Aug 8, 2019
917426c
revert version
jeffdyke Aug 8, 2019
65481b7
add message to Error
jeffdyke Aug 8, 2019
1e7374d
remove the overly verbose implementation and just add a flag to command
jeffdyke Aug 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,5 @@ project/boot
project/build/target
project/plugins/lib_managed
project/plugins/src_managed
project/plugins/target
project/plugins/target
.idea
Copy link
Owner

@sirthias sirthias Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

Copy link
Author

@jeffdyke jeffdyke Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

21 changes: 18 additions & 3 deletions src/main/scala/com/decodified/scalassh/Command.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Owner

@sirthias sirthias Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this a sealed trait.

Copy link
Author

@jeffdyke jeffdyke Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

val command: String
Copy link
Owner

@sirthias sirthias Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@jeffdyke jeffdyke Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

val input: CommandInput = CommandInput.NoInput
val timeout: Option[Int] = None
val safe: Boolean
Copy link
Owner

@sirthias sirthias Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Author

@jeffdyke jeffdyke Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
case class SafeCommand(command: String,
override val input: CommandInput = CommandInput.NoInput,
override val timeout: Option[Int] = None,
safe: Boolean = true)
extends Command
case class UnsafeCommand(command: String,
Copy link
Owner

@sirthias sirthias Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@jeffdyke jeffdyke Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all gone...

override val input: CommandInput = CommandInput.NoInput,
override val timeout: Option[Int] = None,
safe: Boolean = false)
extends Command

object Command {
implicit def fromString(cmd: String): Command = Command(cmd)
def apply(cmd: String, safe: Boolean): Command = if (safe) SafeCommand(cmd) else UnsafeCommand(cmd)
def apply(cmd: String): Command = SafeCommand(cmd)
}

final case class CommandInput(inputStream: Option[InputStream])
Expand Down
9 changes: 6 additions & 3 deletions src/main/scala/com/decodified/scalassh/SshClient.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import net.schmizz.sshj.SSHClient
import net.schmizz.sshj.connection.channel.direct.Session
import net.schmizz.sshj.userauth.keyprovider.KeyProvider
import net.schmizz.sshj.userauth.method.AuthMethod

import scala.io.Source
import scala.util.{Failure, Success, Try}
import scala.util.control.NonFatal
Expand All @@ -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"
Copy link
Owner

@sirthias sirthias Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@jeffdyke jeffdyke Aug 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


def execWithSession(command: Command, session: Session): Try[CommandResult] = {
log.info("Executing SSH command on {}: \"{}\"", Seq(endpoint, command.command): _*)
log.info("Executing SSH command on {}: \"{}\"", Seq(endpoint, logCmd(command)): _*)
protect("Could not execute SSH command on") {
val channel = session.exec(command.command)
command.input.inputStream.foreach(new StreamCopier().copy(_, channel.getOutputStream))
Expand Down Expand Up @@ -155,7 +155,10 @@ final class SshClient(val config: HostConfig) extends ScpTransferable {

protected def protect[T](errorMsg: => String)(f: => T): Try[T] =
try Success(f)
catch { case NonFatal(e) => Failure(SSH.Error(errorMsg + " " + endpoint, e)) }
catch {
case NonFatal(e) =>
Failure(SSH.Error(s"$errorMsg executing on $endpoint - message: ${e.getMessage}", e))
}
}

object SshClient {
Expand Down