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

Disallow Selecting .pub files for SSH & SSL Keys #67

Closed
Jason-Morcos opened this issue Jun 22, 2020 · 8 comments · Fixed by #118
Closed

Disallow Selecting .pub files for SSH & SSL Keys #67

Jason-Morcos opened this issue Jun 22, 2020 · 8 comments · Fixed by #118
Assignees
Labels
Feature Request New feature or request
Projects

Comments

@Jason-Morcos
Copy link
Member

(We really should not let users select .pub files...)

Originally posted by @gboudreau in #52 (comment)

@Jason-Morcos Jason-Morcos added the Feature Request New feature or request label Jun 22, 2020
@Jason-Morcos Jason-Morcos added this to To do in Future Work via automation Jun 22, 2020
@gboudreau
Copy link
Contributor

We can just look in the file, and if we see PRIVATE KEY----- twice, it should be good.

Most ppl use RSA keys, which contain -----BEGIN RSA PRIVATE KEY----- and the equivalent end tag, but I myself use an ED25519 key, which contains -----BEGIN OPENSSH PRIVATE KEY-----. Looking for just the common suffix should be good enough.

@jamesstout
Copy link
Contributor

jamesstout commented Jun 23, 2020

Should be a simple change to this validation:

const char rsaHead[] = "-----BEGIN RSA PRIVATE KEY-----";
const char rsaFoot[] = "-----END RSA PRIVATE KEY-----";
if(FindLinesInFile(file, rsaHead, strlen(rsaHead), rsaFoot, strlen(rsaFoot)))
return YES;

EDIT: No, not as simple as that, SP of course has it's own data/line parsing code....

@jamesstout
Copy link
Contributor

Most ppl use RSA keys, which contain -----BEGIN RSA PRIVATE KEY----- and the equivalent end tag, but I myself use an ED25519 key, which contains -----BEGIN OPENSSH PRIVATE KEY-----. Looking for just the common suffix should be good enough.

What does your cert file have as first/last lines?

@gboudreau
Copy link
Contributor

-----BEGIN OPENSSH PRIVATE KEY-----
...
-----END OPENSSH PRIVATE KEY-----

@jamesstout
Copy link
Contributor

I've updated my branch with some better validation: https://github.com/jamesstout/Sequel-Ace/tree/more-permissions

@Jason-Morcos
Copy link
Member Author

I've updated my branch with some better validation: https://github.com/jamesstout/Sequel-Ace/tree/more-permissions

Looks good to me!
With SecurityScopedBookmarks, do we have to somehow start accessing the specific bookmark when we begin a connection and end it out when we close the connection? How does that work with quitting and reopening the app?

@jamesstout
Copy link
Contributor

With SecurityScopedBookmarks, do we have to somehow start accessing the specific bookmark when we begin a connection and end it out when we close the connection? How does that work with quitting and reopening the app?

Ah, that's a good point, I've forgotten to relinquish access with stopAccessingSecurityScopedResource. In the export controller, I relinquish access when the export ends. We no longer need access.

For the connection controller, we [currently] copy the key to the sandbox, so could call stopAccessingSecurityScopedResource as soon as the key/cert is validated and copied.

However, there was discussion about no longer copying the keys: #52 (comment), so I'd have to figure out where to relinquish access. Maybe after the connection completes. I haven't looked at the actual connection code to see if it retries, or needs the key to keep alive.

I'll have a look later.

@jamesstout
Copy link
Contributor

I've put stopAccessingSecurityScopedResource in _documentWillClose and dealloc. Please see the branch mentioned above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request New feature or request
Projects
No open projects
Future Work
  
Done
Development

Successfully merging a pull request may close this issue.

4 participants