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.

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

[SSHD-700] Fix the issues of the agent forwarding implementation of IETF. #34

Closed
wants to merge 5 commits into from
Closed

Conversation

lifangning
Copy link
Contributor

[SSHD-700] Fix the issues of the agent forwarding implementation of IETF.

public static final byte SSH_AGENT_LIST_KEYS = (byte) 204;
public static final byte SSH_AGENT_PRIVATE_KEY_OP = (byte) 205;
// Messages sent by the agent
public static final byte SSH_AGENT_KEY_LIST = 104;
Copy link
Contributor

Choose a reason for hiding this comment

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

These values must be int since byte is only [-127..127]..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make changes based on the suggestions.

Buffer buffer = createBuffer(SshAgentConstants.SSH2_AGENTC_REQUEST_IDENTITIES, 1);
byte cmd = SshAgentConstants.SSH2_AGENTC_REQUEST_IDENTITIES;
byte okcmd = SshAgentConstants.SSH2_AGENT_IDENTITIES_ANSWER;
if (FactoryManager.AGENT_FORWARDING_TYPE_IETF.equals(channelType)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use int instead of byte to allow for values > 127...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make changes based on the suggestions.

buffer = request(prepare(buffer));
int type = buffer.getUByte();
if (type != SshAgentConstants.SSH2_AGENT_IDENTITIES_ANSWER) {
if (type != okcmd) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not seem equivalent to previous code - you are creating a new buffer whereas the previous code was not doing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous code also create a new buffer (line 74).

Buffer buffer = createBuffer(SshAgentConstants.SSH2_AGENTC_SIGN_REQUEST);
byte cmd = SshAgentConstants.SSH2_AGENTC_SIGN_REQUEST;
byte okcmd = SshAgentConstants.SSH2_AGENT_SIGN_RESPONSE;
if (FactoryManager.AGENT_FORWARDING_TYPE_IETF.equals(channelType)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use int to allow for values > 127

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make changes based on the suggestions.

Buffer buffer = createBuffer(cmd);
if (FactoryManager.AGENT_FORWARDING_TYPE_IETF.equals(channelType)) {
buffer.putString("sign");
}
buffer.putPublicKey(key);
Copy link
Contributor

Choose a reason for hiding this comment

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

The code does not seem equivalent to previous one - there is now a new created buffer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous code also create a new buffer (line 102).

KeyUtils.getKeyType(key), KeyUtils.getFingerPrint(key),
algorithm, BufferUtils.toHex(':', signature));
byte[] signature = null;
if (FactoryManager.AGENT_FORWARDING_TYPE_IETF.equals(channelType)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't initialize signature to null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make changes based on the suggestions.

@@ -49,7 +48,7 @@ public ProxyAgentFactory() {

@Override
public List<NamedFactory<Channel>> getChannelForwardingFactories(FactoryManager manager) {
return UnixAgentFactory.DEFAULT_FORWARDING_CHANNELS;
return LocalAgentFactory.DEFAULT_FORWARDING_CHANNELS;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this depends on the O/S type ? E.g:

return OsUtils.isUnix() ? UnixAgentFactory.DEFAULT_FORWARDING_CHANNELS : LocalAgentFactory.DEFAULT_FORWARDING_CHANNELS;

@lgoldstein
Copy link
Contributor

Merged - please close the request

@lifangning lifangning closed this Jul 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants