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

[SSHD-1273] Add support to use env vars together with subsystem #230

Merged
merged 1 commit into from Jul 1, 2022
Merged

[SSHD-1273] Add support to use env vars together with subsystem #230

merged 1 commit into from Jul 1, 2022

Conversation

andreid911
Copy link
Contributor

No description provided.

@andreid911 andreid911 changed the title Add support to use env vars together with subsystem [SSHD-1273] Add support to use env vars together with subsystem Jul 1, 2022
@andreid911
Copy link
Contributor Author

@tomaswolf @lgoldstein could you please take a look.

@andreid911 andreid911 requested a review from tomaswolf July 1, 2022 14:50
lgoldstein
lgoldstein suggested changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@lgoldstein lgoldstein left a comment

Choose a reason for hiding this comment

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

Looks fine - a few minor comments

sshd-core/src/main/java/org/apache/sshd/client/channel/ChannelSession.java Show resolved Hide resolved
sshd-core/src/main/java/org/apache/sshd/client/channel/ChannelSession.java Outdated Show resolved Hide resolved
sshd-core/src/main/java/org/apache/sshd/client/channel/PtyCapableChannelSession.java Show resolved Hide resolved
tomaswolf
tomaswolf reviewed Jul 1, 2022
View reviewed changes
Copy link
Member

@tomaswolf tomaswolf left a comment

Choose a reason for hiding this comment

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

Looks good to me. Setting environment variables has got nothing to do with terminals. Can be merged once the CI builds succeed.

Note that this change theoretically also enables setting environment variables on a SftpChannelSubsystem (but it can't be used there in the default implementation because the DefaultSftpClient creates the channel and then opens it right away).

@andreid911 andreid911 requested a review from tomaswolf July 1, 2022 18:39
@tomaswolf tomaswolf merged commit 01cab3e into apache:master Jul 1, 2022
@lgoldstein
Copy link
Contributor

RFC 4254 - section 6.4 mentions shell and/or command but not subsystem. However, I see no harm in allowing this for subsystems as well. IMO it is the user's responsibility to make sure that the peer handles environment settings correctly. Our code simply updates a StandardEnvironment instance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lgoldstein lgoldstein lgoldstein requested changes

@tomaswolf tomaswolf Awaiting requested review from tomaswolf

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@andreid911 @lgoldstein @tomaswolf