search

roddhjav / pass-import

A pass extension for importing data from most existing password managers
https://www.passwordstore.org/
GNU General Public License v3.0
800 stars 88 forks source link

added exporter for pwdsphinx #183

Closed stef closed 2 years ago

stef commented 2 years ago

This PR introduces support for pwdsphinx: https://www.ctrlc.hu/~stef/blog/posts/sphinx.html

SPHINX is an online "password store" that follows the KISS principle and has stronger than usual security guarantees. It is not possible to export passwords from SPHINX due to there not being any native support for listing all records belonging to a user. Thus thus this PR introduces only an exporter to SPHINX this causes some trouble as in https://github.com/stef/pass-import/blob/master/CONTRIBUTING.md#how-to-add-the-support-for-a-new-password-manager mandates:

Add a file named tests/assets/db/mymanager[.csv,.xml,...]. No contribution will be accepted without this file. This file must contain the exported data from your manager.

Since there is not export functionality possible with SPHINX it is impossible to provide such a file. I hope despite this you accept this PR as it is not really a violation of the above mandate since such a file can only be demanded for importers not exporters. (huh it's kinda confusing what is an export and what an import, it all depends on the perspective i guess).

thanks a lot for your awesome project.

stef commented 2 years ago

hrmpf. CI obviously fails since i did not add pwdsphinx as a dep in travis/gitlab CI config. the problem is pwdsphinx depends on two libs which are not yet in debian (but are already packaged only need to be added to unstable).

i understand if the lack of deps is a blocking reason for acceptance of this PR.

i could provide the two libs online for download into the CI VMsif that would be acceptable?

stef commented 2 years ago

i managed to mock/fake the missing libs in travis-ci, however at the cost of moving to bionic from xenial and going from py3.6 to py3.7, i dunno if that is acceptable. i do hope it is. i have not touched the .gitlab-ci config, that if there is a gitlab hosting this somewhere will probably fail.

roddhjav commented 2 years ago

Hi, Thank a lot for this PR. I will merge it, just allow me a bit of time (pwdsphinx seems quite neat, I want to test it too).

Meanwhile, I have a few comments on your PR.

(huh it's kinda confusing what is an export and what an import, it all depends on the perspective i guess).

You are right. In pass-import jargon an importer is a source password manager whereas an exported is a destination password manager. The jargon comes from the time pass was the only destination pm possible and therefore all other password manager were importers.

Since there is not export functionality possible with SPHINX it is impossible to provide such a file.

No problem with the actual file (that more for CSV and similar exported file). But it seems possible to export the data from sphinx to another password manager simply because you can list and get the data stored in sphinx. So this integration would be nice to add.

stef commented 2 years ago

Hi, Thank a lot for this PR. I will merge it, just allow me a bit of time (pwdsphinx seems quite neat, I want to test it too).

thanks a lot!

Meanwhile, I have a few comments on your PR.

* As it is not a common format (like in XML, CSV...) you should remove `pass_import/formats/sphinx.py ` and put all the logic in `pass_import/managers/sphinx.py`.

ack. makes sense. will do so.

* Looking at the test result, I see you only have 73% of code coverage. It indicate you might need to add more tests. Note you don't need 100%, but 73% is very low.

right. will add more tests.

Since there is not export functionality possible with SPHINX it is impossible to provide such a file.

No problem with the actual file (that more for CSV and similar exported file). But it seems possible to export the data from sphinx to another password manager simply because you can list and get the data stored in sphinx. So this integration would be nice to add.

listing is not quite possible. you have to know the "hostnames" or "urls" for which you want to query the known users names. but the client itself does not handle any such lists of hostnames/urls/whatever. since that would mean the client has to sync this information to all other devices, and the whole idea is to not have to sync anything. so yes in theory if you give sphinx a hostname like "github.com" it will give you a list of usernames associated with you and github.com, which you then can query individually, but there is no complete list of all hosts/urls/whatever. although i also have to say that in the contribs there's some x11 integration with dmenu which caches hostnames, but that is independent of the client itself.

i'll update the PR ASAP with moving format to manager and adding more tests.

thanks for your comments.

stef commented 2 years ago

i updated the PR according to your comments.

stef commented 2 years ago

hmmm, i don't quite see why the build is failing here, i don't think it is related to my changes.

roddhjav commented 2 years ago

is there a way to have pass-audit being used during a import?

Yes, I can call pass-audit from pass-import like

i don't quite see why the build is failing here, i don't think it is related to my changes.

It is not related to your last changes but it is linked with your branch. The master branch still runs fine.

stef commented 2 years ago

is there a way to have pass-audit being used during a import?

Yes, I can call pass-audit from pass-import like

i searched in this repo for references for audit, but couldn't find any good explanation. how can audit be used during an import?

i don't quite see why the build is failing here, i don't think it is related to my changes.

It is not related to your last changes but it is linked with your branch. The master branch still runs fine.

weird.

roddhjav commented 2 years ago

A have now added the support password audit. See caf32e5b220f9bc52a9ea5bf276297829d6c6bbc

stef commented 2 years ago

that is so awesome! <3 <3 <3 thanks

roddhjav commented 2 years ago

Thanks. Merged!