Skip to content

Conversation

@jindrichskupa
Copy link
Contributor

@jindrichskupa jindrichskupa commented Aug 26, 2021

  • Skip private key in case of error (encrypted without passphrase, wrong format, ...)
  • Close open reader/writer on ssh channel when finish or error occurs
  • Don't fail but create new empty config when no config (empty string) file was used

@jindrichskupa
Copy link
Contributor Author

@davrodpin please let me know your opinion

@davrodpin davrodpin added the enhancement New feature or request label Aug 26, 2021
tunnel/config.go Outdated
// NewSSHConfigFile creates a new instance of SSHConfigFile based on the
// ssh config file from configPath
func NewSSHConfigFile(configPath string) (*SSHConfigFile, error) {
if configPath == "" {
Copy link
Owner

Choose a reason for hiding this comment

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

How about returning a specific error message explaining what the error is about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Empty config path is not an error, I don't want to use existing config, I want to use the empty one. Maybe, teh better solution is update the code above to skip reading of existing config not here with error.

Copy link
Owner

Choose a reason for hiding this comment

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

Please see my previous comment on how to allow cfgPath to be empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, thanks for your review. I will update the code later this week.

tunnel/tunnel.go Outdated
if cfgPath == "" {
c = NewEmptySSHConfigStruct()
} else {
c, err := NewSSHConfigFile(cfgPath)
Copy link
Owner

@davrodpin davrodpin Sep 14, 2021

Choose a reason for hiding this comment

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

By using :=, the code is creating a new c variable rather than using the one defined outside of the else scope. Please use =. This is also causing the app to fail when building.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:= must be used, because err is not defined, so I defined it.

Copy link
Owner

Choose a reason for hiding this comment

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

You can do something like:

} else {
  var err error

  c, err = NewSSHConfigFile(cfgPath)

...

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 defined var err error above. Should I move it here?

@davrodpin
Copy link
Owner

Hi @jindrichskupa, changes are all good but the fix you made to close the reader and writer on tunnel.copyConn exposed another problem which is an eventual attempt to close the tunnel while it is trying to reconnect hence the unit tests are failing.

I've provided a fix and it is merged to master. Could you please update your branch with the latest master?

Thanks!

David Pinheiro and others added 12 commits September 17, 2021 13:48
This changes adds a new command to show the runtime configuration of one or all
running instances of mole running on the system.
It leverages the internal rpc server to call a remote procedure that
returns the runtime configuration.
This change makes the runtime information to return the addresses used
by the ssh channels instead of the input.
This change makes the connection to the ssh server and channels setup to
happen in a goroutine when mole is trying to reconnect.
The reason for this change is to allow the tunnel to be closed while the
attemp to reconnect is still in progress.
@jindrichskupa
Copy link
Contributor Author

Should be done. Thanks for you patience.

@davrodpin
Copy link
Owner

@jindrichskupa, thank you very much for your contribution!

@davrodpin davrodpin merged commit 045aee0 into davrodpin:master Sep 17, 2021
@jindrichskupa jindrichskupa deleted the cookielab-fixes branch September 18, 2021 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants