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

Add Config #34

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

Add Config #34

wants to merge 6 commits into from

Conversation

SmsS4
Copy link

@SmsS4 SmsS4 commented Jan 30, 2022

This pr is to solve #31

@Dentrax
Copy link

Dentrax commented Sep 16, 2023

Thanks for this! @SmsS4

I tried to use NewWithConfigFile() function with goph.NewConn() but got open ~/.ssh/identity: no such file or directory error. Any ideas? 🤔

@SmsS4
Copy link
Author

SmsS4 commented Sep 16, 2023

hmmmm this PRis for 1.5 years ago
I will check it tomorrow :)

@SmsS4 SmsS4 closed this Sep 16, 2023
@SmsS4 SmsS4 reopened this Sep 16, 2023
@Dentrax
Copy link

Dentrax commented Sep 16, 2023

Thanks for prompt reply! Also it'd be nice to add another function to return only Auth:

func GetAuth(host string) (goph.Auth, error) {
	auth, err := goph.Key(ssh_config.Get(host, "IdentityFile"), "")
	if err != nil {
		return nil, fmt.Errorf("get key: %w", err)
	}
	return auth, nil
}

Copy link
Owner

@melbahja melbahja left a comment

Choose a reason for hiding this comment

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

Hey thanks for the PR, can you please use return instead of log fatal.

client.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
@SmsS4
Copy link
Author

SmsS4 commented Sep 17, 2023

I fixed your comments

@SmsS4 SmsS4 requested a review from melbahja September 17, 2023 21:06
@SmsS4
Copy link
Author

SmsS4 commented Sep 17, 2023

Thanks for prompt reply! Also it'd be nice to add another function to return only Auth:

func GetAuth(host string) (goph.Auth, error) {
	auth, err := goph.Key(ssh_config.Get(host, "IdentityFile"), "")
	if err != nil {
		return nil, fmt.Errorf("get key: %w", err)
	}
	return auth, nil
}

I used your code in new commit
thanks

By default ssh_config.Get(host, "IdentityFile") returns ~/.ssh/identity so you have to set IdentityFile in ssh config.
for example:

Host vpn
	HostName 127.0.0.1
	User test
	IdentityFile /home/demol/.ssh/id_rsa

@melbahja
Copy link
Owner

Hey thanks for the update,

one more thing if you can use NewConn instead of New in case of custom port with fallback to 22.

v2 of this package I should add more configuration options suport

@Dentrax
Copy link

Dentrax commented Sep 18, 2023

By default ssh_config.Get(host, "IdentityFile") returns ~/.ssh/identity so you have to set IdentityFile in ssh config.

I was wondering if it does make sense to return an error in case if IdentityFile is empty or equals to ssh_config.Default("IdentityFile")

Also you'd consider to remove leading and trailing "s, I just filed an issue for that: kevinburke/ssh_config#61

strings.Trim(ssh_config.Get(host, key), "\"")

@SmsS4
Copy link
Author

SmsS4 commented Sep 18, 2023

I will update my fork and use NewConn instead of New (probably tomorrow or day after tomorrow
Thanks @melbahja

Hey thanks for the update,

one more thing if you can use NewConn instead of New in case of custom port with fallback to 22.

v2 of this package I should add more configuration options suport

@SmsS4
Copy link
Author

SmsS4 commented Sep 18, 2023

By default ssh_config.Get(host, "IdentityFile") returns ~/.ssh/identity so you have to set IdentityFile in ssh config.

I was wondering if it does make sense to return an error in case if IdentityFile is empty or equals to ssh_config.Default("IdentityFile")

Also you'd consider to remove leading and trailing "s, I just filed an issue for that: kevinburke/ssh_config#61

strings.Trim(ssh_config.Get(host, key), "\"")

It's probably good idea to use ~/.ssh/id_rsa by default and not ~/.ssh/identity but I only return error if ~/.ssh/identity doesn't exists.
I will update this soon @Dentrax

@SmsS4
Copy link
Author

SmsS4 commented Sep 20, 2023

@melbahja
I used NewConn instead of New with a little bit of refactoring

@melbahja
Copy link
Owner

I'll need a bit of time to test the changes. Thanks.

@mehrdad-khojastefar
Copy link

Can you review these changes ? this pr could really help us.

@melbahja
Copy link
Owner

The ssh_config lib has many issues like:

I think you should solve the ssh config problem outside of goph and just pass the hostname, port, and identity to it, and I don't think this is a good think to add it to v1 maybe in v2.

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.

4 participants