Skip to content
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

Jump to bottom

Issue127 #128

Merged
merged 17 commits into from
Mar 8, 2022
Merged

Issue127 #128

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
f7f7be8
allow the setting of a logger to the JSch instance to allow
kimmerin Feb 17, 2022
fee4dd7
reenabled message
Feb 17, 2022
f8220b3
changes according to the feedback in the pull request
Feb 17, 2022
f71753e
removed commented out code containing the original exception without
Feb 17, 2022
7c2f3e1
removed method unrelated to this issue
Feb 17, 2022
ffe145d
added missing bracket
Feb 17, 2022
c156b66
changed logging in compression classes to use own methods
Feb 17, 2022
8de3843
removed whitespaces
Feb 18, 2022
6431bcf
get logger from jsch on instantiation and allow to set a different one
Feb 18, 2022
48a6ff2
changed logger setting to fall back to JSch-logger retrieval if no
Feb 18, 2022
1cc013c
added test for logger handling
Feb 18, 2022
af03841
added javadoc
Feb 18, 2022
b127caf
check if logging is necessary before calling
Feb 22, 2022
55e3ef3
Implementation of Issue 127: Allow the setting of a logger
kimmerin Feb 17, 2022
636d0d1
Merge branch 'issue127' of https://github.com/kimmerin/jsch into issu…
kimmerin Feb 22, 2022
77a878e
set JSch static logger to null to make sure that we start at a known
kimmerin Mar 6, 2022
c7d1466
Keep the logger we find at the beginning of the test and reset it in
kimmerin Mar 6, 2022
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
31 changes: 18 additions & 13 deletions src/main/java/com/jcraft/jsch/Channel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,38 +45,43 @@ public abstract class Channel implements Runnable{

static int index=0;
private static Vector<Channel> pool=new Vector<>();
static Channel getChannel(String type){
static Channel getChannel(String type, Session session){
Channel ret = null;
if(type.equals("session")){
return new ChannelSession();
ret = new ChannelSession();
Copy link
Contributor

@norrisjeremy norrisjeremy Feb 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if we should perhaps we just add a Session session argument to each of the constructors of the ChannelXXX implementations instead of adding an explicit setSession(session) call below?
That would then allow us to remove the addChannel() method in Session & PortWatcher classes I think?
@mwiede any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've solved it this way to keep the number of affected classes as low as possible. As far as I can see, the solution with setChannel() (that is implemented in the abstract class Channel) would allow you to do the same, or do I miss something here?

}
if(type.equals("shell")){
return new ChannelShell();
ret = new ChannelShell();
}
if(type.equals("exec")){
return new ChannelExec();
ret = new ChannelExec();
}
if(type.equals("x11")){
return new ChannelX11();
ret = new ChannelX11();
}
if(type.equals("auth-agent@openssh.com")){
return new ChannelAgentForwarding();
ret = new ChannelAgentForwarding();
}
if(type.equals("direct-tcpip")){
return new ChannelDirectTCPIP();
ret = new ChannelDirectTCPIP();
}
if(type.equals("forwarded-tcpip")){
return new ChannelForwardedTCPIP();
ret = new ChannelForwardedTCPIP();
}
if(type.equals("sftp")){
return new ChannelSftp();
ret = new ChannelSftp();
}
if(type.equals("subsystem")){
return new ChannelSubsystem();
ret = new ChannelSubsystem();
}
if(type.equals("direct-streamlocal@openssh.com")){
return new ChannelDirectStreamLocal();
ret = new ChannelDirectStreamLocal();
}
return null;
if (ret == null) {
return null;
}
ret.setSession(session);
return ret;
}
static Channel getChannel(int id, Session session){
synchronized(pool){
Expand Down Expand Up @@ -118,7 +123,7 @@ static void del(Channel c){
volatile int reply=0;
volatile int connectTimeout=0;

private Session session;
protected Session session;

int notifyme=0;

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/jcraft/jsch/ChannelDirectStreamLocal.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public class ChannelDirectStreamLocal extends ChannelDirectTCPIP {
protected Packet genChannelOpenPacket() {

if (socketPath == null) {
JSch.getLogger().log(Logger.FATAL, "socketPath must be set");
session.getLogger().log(Logger.FATAL, "socketPath must be set");
throw new RuntimeException("socketPath must be set");
}

Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/jcraft/jsch/ChannelForwardedTCPIP.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,8 @@ void getData(Buffer buf){
this.config = getPort(_session, null, port);

if(this.config == null){
if(JSch.getLogger().isEnabled(Logger.ERROR)){
JSch.getLogger().log(Logger.ERROR,
if(_session.getLogger().isEnabled(Logger.ERROR)){
_session.getLogger().log(Logger.ERROR,
"ChannelForwardedTCPIP: "+Util.byte2str(addr)+":"+port+" is not registered.");
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/jcraft/jsch/Compression.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
public interface Compression{
static public final int INFLATER=0;
static public final int DEFLATER=1;
void init(int type, int level) throws Exception;
void init(Session session, int type, int level) throws Exception;
Copy link
Contributor

Choose a reason for hiding this comment

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

In #126 I've attempted to restore the backwards incompatibilities I introduced in several public interfaces.

Could we change revert this method signature back, and then instead add something like this?

default void init(int type, int level, Session session) throws Exception {init(type, level);}
default Logger getLogger() {return JSch.getLogger();}

That way this interface maintains backwards compatibility.
We can thin change the built-in implementations to override these methods as appropriate, but any users that are using custom Compression implementations don't suddenly have their implementations break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted the change but solved it a bit differently to keep code duplication in juz.Compression and jzlib.Compression to a minimum. There is now a default setSession-method doing nothing and a static logMessage in Session with a Supplier that is now called in both Compression-implementations.

byte[] compress(byte[] buf, int start, int[] len);
byte[] uncompress(byte[] buf, int start, int[] len);
}
7 changes: 3 additions & 4 deletions src/main/java/com/jcraft/jsch/DHECN.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ public abstract class DHECN extends KeyExchange{
@Override
public void init(Session session,
byte[] V_S, byte[] V_C, byte[] I_S, byte[] I_C) throws Exception{
this.session=session;
this.V_S=V_S;
this.V_C=V_C;
this.I_S=I_S;
Expand Down Expand Up @@ -94,10 +93,10 @@ public void init(Session session,

session.write(packet);

if(JSch.getLogger().isEnabled(Logger.INFO)){
JSch.getLogger().log(Logger.INFO,
if(session.getLogger().isEnabled(Logger.INFO)){
session.getLogger().log(Logger.INFO,
"SSH_MSG_KEX_ECDH_INIT sent");
JSch.getLogger().log(Logger.INFO,
session.getLogger().log(Logger.INFO,
"expecting SSH_MSG_KEX_ECDH_REPLY");
}

Expand Down
13 changes: 6 additions & 7 deletions src/main/java/com/jcraft/jsch/DHGEX.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ public class DHGEX extends KeyExchange{
@Override
public void init(Session session,
byte[] V_S, byte[] V_C, byte[] I_S, byte[] I_C) throws Exception{
this.session=session;
this.V_S=V_S;
this.V_C=V_C;
this.I_S=I_S;
Expand Down Expand Up @@ -101,10 +100,10 @@ public void init(Session session,
buf.putInt(max);
session.write(packet);

if(JSch.getLogger().isEnabled(Logger.INFO)){
JSch.getLogger().log(Logger.INFO,
if(session.getLogger().isEnabled(Logger.INFO)){
session.getLogger().log(Logger.INFO,
"SSH_MSG_KEX_DH_GEX_REQUEST("+min+"<"+preferred+"<"+max+") sent");
JSch.getLogger().log(Logger.INFO,
session.getLogger().log(Logger.INFO,
"expecting SSH_MSG_KEX_DH_GEX_GROUP");
}

Expand Down Expand Up @@ -144,10 +143,10 @@ public boolean next(Buffer _buf) throws Exception{
buf.putMPInt(e);
session.write(packet);

if(JSch.getLogger().isEnabled(Logger.INFO)){
JSch.getLogger().log(Logger.INFO,
if(session.getLogger().isEnabled(Logger.INFO)){
session.getLogger().log(Logger.INFO,
"SSH_MSG_KEX_DH_GEX_INIT sent");
JSch.getLogger().log(Logger.INFO,
session.getLogger().log(Logger.INFO,
"expecting SSH_MSG_KEX_DH_GEX_REPLY");
}

Expand Down
7 changes: 3 additions & 4 deletions src/main/java/com/jcraft/jsch/DHGN.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ public abstract class DHGN extends KeyExchange{
@Override
public void init(Session session,
byte[] V_S, byte[] V_C, byte[] I_S, byte[] I_C) throws Exception{
this.session=session;
this.V_S=V_S;
this.V_C=V_C;
this.I_S=I_S;
Expand Down Expand Up @@ -101,10 +100,10 @@ public void init(Session session,

session.write(packet);

if(JSch.getLogger().isEnabled(Logger.INFO)){
JSch.getLogger().log(Logger.INFO,
if(session.getLogger().isEnabled(Logger.INFO)){
session.getLogger().log(Logger.INFO,
"SSH_MSG_KEXDH_INIT sent");
JSch.getLogger().log(Logger.INFO,
session.getLogger().log(Logger.INFO,
"expecting SSH_MSG_KEXDH_REPLY");
}

Expand Down
7 changes: 3 additions & 4 deletions src/main/java/com/jcraft/jsch/DHXEC.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ public abstract class DHXEC extends KeyExchange{
@Override
public void init(Session session,
byte[] V_S, byte[] V_C, byte[] I_S, byte[] I_C) throws Exception{
this.session=session;
this.V_S=V_S;
this.V_C=V_C;
this.I_S=I_S;
Expand Down Expand Up @@ -99,10 +98,10 @@ public void init(Session session,

session.write(packet);

if(JSch.getLogger().isEnabled(Logger.INFO)){
JSch.getLogger().log(Logger.INFO,
if(session.getLogger().isEnabled(Logger.INFO)){
session.getLogger().log(Logger.INFO,
"SSH_MSG_KEX_ECDH_INIT sent");
JSch.getLogger().log(Logger.INFO,
session.getLogger().log(Logger.INFO,
"expecting SSH_MSG_KEX_ECDH_REPLY");
}

Expand Down
16 changes: 14 additions & 2 deletions src/main/java/com/jcraft/jsch/JSch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public class JSch{
/**
* The version number.
*/
public static final String VERSION = Version.getVersion();
public static final String VERSION = "0.2.0"; //Version.getVersion();
Copy link
Contributor

Choose a reason for hiding this comment

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

This part needs to be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That part wasn't intended to be in the commit


static Hashtable<String, String> config=new Hashtable<>();
static{
Expand Down Expand Up @@ -280,13 +280,14 @@ public void setConfigRepository(ConfigRepository configRepository) {

private HostKeyRepository known_hosts=null;

private static final Logger DEVNULL=new Logger(){
static final Logger DEVNULL=new Logger(){
@Override
public boolean isEnabled(int level){return false;}
@Override
public void log(int level, String message){}
};
static Logger logger=DEVNULL;
private Logger instLogger;

public JSch(){
}
Expand Down Expand Up @@ -682,6 +683,17 @@ public static void setLogger(Logger logger){
JSch.logger=logger;
}

public Logger getInstanceLogger() {
if (this.instLogger == null) {
return logger;
}
return instLogger;
}

public void setInstanceLogger(Logger logger) {
this.instLogger = logger;
}

public static Logger getLogger(){
return logger;
}
Expand Down
37 changes: 21 additions & 16 deletions src/main/java/com/jcraft/jsch/KeyExchange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@ public abstract class KeyExchange{

public abstract void init(Session session,
byte[] V_S, byte[] V_C, byte[] I_S, byte[] I_C) throws Exception;
public void doInit(Session session,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should make this public. I'd change it to package private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it. I've copied init's method signature to make sure that the parameters are identical.

byte[] V_S, byte[] V_C, byte[] I_S, byte[] I_C) throws Exception {
this.session = session;
init(session, V_S, V_C, I_S, I_C);
}
public abstract boolean next(Buffer buf) throws Exception;

public abstract int getState();
Expand Down Expand Up @@ -96,13 +101,13 @@ protected static String[] guess(Session session, byte[]I_S, byte[]I_C) throws Ex
Buffer sb=new Buffer(I_S); sb.setOffSet(17);
Buffer cb=new Buffer(I_C); cb.setOffSet(17);

if(JSch.getLogger().isEnabled(Logger.INFO)){
if(session.getLogger().isEnabled(Logger.INFO)){
for(int i=0; i<PROPOSAL_MAX; i++){
JSch.getLogger().log(Logger.INFO,
session.getLogger().log(Logger.INFO,
"kex: server: "+Util.byte2str(sb.getString()));
}
for(int i=0; i<PROPOSAL_MAX; i++){
JSch.getLogger().log(Logger.INFO,
session.getLogger().log(Logger.INFO,
"kex: client: "+Util.byte2str(cb.getString()));
}
sb.setOffSet(17);
Expand Down Expand Up @@ -164,17 +169,17 @@ else if(guess[i]==null){
throw new JSchException(e.toString(), e);
}

if(JSch.getLogger().isEnabled(Logger.INFO)){
JSch.getLogger().log(Logger.INFO,
if(session.getLogger().isEnabled(Logger.INFO)){
session.getLogger().log(Logger.INFO,
"kex: algorithm: "+guess[PROPOSAL_KEX_ALGS]);
JSch.getLogger().log(Logger.INFO,
session.getLogger().log(Logger.INFO,
"kex: host key algorithm: "+guess[PROPOSAL_SERVER_HOST_KEY_ALGS]);
JSch.getLogger().log(Logger.INFO,
session.getLogger().log(Logger.INFO,
"kex: server->client"+
" cipher: "+guess[PROPOSAL_ENC_ALGS_STOC]+
" MAC: "+(_s2cAEAD?("<implicit>"):(guess[PROPOSAL_MAC_ALGS_STOC]))+
" compression: "+guess[PROPOSAL_COMP_ALGS_STOC]);
JSch.getLogger().log(Logger.INFO,
session.getLogger().log(Logger.INFO,
"kex: client->server"+
" cipher: "+guess[PROPOSAL_ENC_ALGS_CTOS]+
" MAC: "+(_c2sAEAD?("<implicit>"):(guess[PROPOSAL_MAC_ALGS_CTOS]))+
Expand Down Expand Up @@ -255,8 +260,8 @@ protected boolean verify(String alg, byte[] K_S, int index,
sig.update(H);
result=sig.verify(sig_of_H);

if(JSch.getLogger().isEnabled(Logger.INFO)){
JSch.getLogger().log(Logger.INFO,
if(session.getLogger().isEnabled(Logger.INFO)){
session.getLogger().log(Logger.INFO,
"ssh_rsa_verify: "+foo+" signature "+result);
}
}
Expand Down Expand Up @@ -300,8 +305,8 @@ else if(alg.equals("ssh-dss")){
sig.update(H);
result=sig.verify(sig_of_H);

if(JSch.getLogger().isEnabled(Logger.INFO)){
JSch.getLogger().log(Logger.INFO,
if(session.getLogger().isEnabled(Logger.INFO)){
session.getLogger().log(Logger.INFO,
"ssh_dss_verify: signature "+result);
}
}
Expand Down Expand Up @@ -345,8 +350,8 @@ else if(alg.equals("ecdsa-sha2-nistp256") ||

result=sig.verify(sig_of_H);

if(JSch.getLogger().isEnabled(Logger.INFO)){
JSch.getLogger().log(Logger.INFO,
if(session.getLogger().isEnabled(Logger.INFO)){
session.getLogger().log(Logger.INFO,
"ssh_ecdsa_verify: "+alg+" signature "+result);
}
}
Expand Down Expand Up @@ -378,8 +383,8 @@ else if(alg.equals("ssh-ed25519") ||

result=sig.verify(sig_of_H);

if(JSch.getLogger().isEnabled(Logger.INFO)){
JSch.getLogger().log(Logger.INFO,
if(session.getLogger().isEnabled(Logger.INFO)){
session.getLogger().log(Logger.INFO,
"ssh_eddsa_verify: "+alg+" signature "+result);
}
}
Expand Down
9 changes: 9 additions & 0 deletions src/main/java/com/jcraft/jsch/KeyPair.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,15 @@ public void writePrivateKey(OutputStream out, byte[] passphrase){

abstract byte[] getKeyTypeName();
public abstract int getKeyType();

public String getKeyTypeName(String encoding) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This part seems unrelated to the logging changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is. Shouldn't be part of the commit, either.

try {
return new String(getKeyTypeName(), encoding);
}
catch(UnsupportedEncodingException uee) {
return null;
}
}

/**
* Returns the blob of the public key.
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/jcraft/jsch/KeyPairDeferred.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public boolean decrypt(byte[] _passphrase) {
return true;
}
if (_passphrase == null) {
JSch.getLogger().log(Logger.ERROR, "no passphrase set.");
jsch.getInstanceLogger().log(Logger.ERROR, "no passphrase set.");
return false;
}

Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/jcraft/jsch/KeyPairPKCS8.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,15 +370,15 @@ else if(Util.array_equals(id, aes256cbc)){
cipher=c.getDeclaredConstructor().newInstance();
}
catch(Exception e){
if(JSch.getLogger().isEnabled(Logger.FATAL)){
if(jsch.getInstanceLogger().isEnabled(Logger.FATAL)){
String message="";
if(name==null){
message="unknown oid: "+Util.toHex(id);
}
else {
message="function "+name+" is not supported";
}
JSch.getLogger().log(Logger.FATAL, "PKCS8: "+message);
jsch.getInstanceLogger().log(Logger.FATAL, "PKCS8: "+message);
}
}
return cipher;
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/com/jcraft/jsch/PortWatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ public void run(){
OutputStream out=socket.getOutputStream();
if(socketPath!=null && socketPath.length()>0){
ChannelDirectStreamLocal channel = new ChannelDirectStreamLocal();
channel.setSession(session);
channel.init();
channel.setInputStream(in);
channel.setOutputStream(out);
Expand All @@ -211,7 +212,9 @@ public void run(){
channel.connect(connectTimeout);
} else {
ChannelDirectTCPIP channel = new ChannelDirectTCPIP();
channel.setSession(session);
channel.init();
channel.setSession(session);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have to call setSession() twice here (both before and after init()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't be. I'll remove the second one (I suppose while going over the changes I've missed the first setSession I've already added and added the second one).

channel.setInputStream(in);
channel.setOutputStream(out);
session.addChannel(channel);
Expand Down
Loading