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

Minor example modification leads to nonce assertion error #23

Open
8-bit-fox opened this issue Oct 2, 2020 · 9 comments
Open

Minor example modification leads to nonce assertion error #23

8-bit-fox opened this issue Oct 2, 2020 · 9 comments

Comments

@8-bit-fox
Copy link

Instead of failing hard in L20, I would like to wait for the database to be opened and then proceed. Therefore instead of L20-L25 I introduced

    if not c.is_database_open(connection_id):
        c.wait_for_unlock()

However, one of the subsequent calls c.associate or c.get_login (I omit the set_login part) fails with

assert response_nonce == expected_nonce
AssertionError

indicating that something in the nonce calculation went south. When I execute the script over and over again, sometimes the script even succeeds (but not very often).

Since I am not firm with the protocol I am asking for help to fix this as I have no clue where to start looking.

Best regards

@hrehfeld
Copy link
Owner

hrehfeld commented Oct 3, 2020 via email

@8-bit-fox
Copy link
Author

8-bit-fox commented Oct 3, 2020

Can you give a full example of your code?

Sure. https://gist.github.com/marquardts/d19cef9193f8ac929e4e8f3f5bf634b2

What are you trying to achieve?

In case of a locked DB (I use an autolock) I want that the scripts holds execution until the database is unlocked. Then it should continue to retrieve the credentials.
Currently, in the example it fails by throwing an exception. Of course, I could write a wrapper that catches the exception, waits for user input and then restarts the whole process. But that would be very ugly.

@hrehfeld
Copy link
Owner

hrehfeld commented Oct 6, 2020

Well, not sure what's going on, but why don't you just catch the exception and loop with a timer until you c.get_database_hash(id)?

@8-bit-fox
Copy link
Author

Sure, that's possible. But AFAIU the whole point of c.wait_for_unlock() is to receive a notification instead of flushing KPXC with requests. So, in terms of design I like the implemented method more - if it would work. Therefore I asked for help 🙂

@piegamesde
Copy link
Contributor

I probably have exactly you use case, even our scripts look more or less the same ^^. I'm not sure why it fails for you.

I remember having issues with the script failing when started before the database was unlocked (or something like that, vaguely remember). But I don't get that error anymore and thus never bothered. Maybe it's a bug in older Keepass? (I'm on 2.6.1-snapshot atm, probably a version from around May or so).

P.S.: You might also like this ;-)

P.P.S.: If that use case is more common, maybe we'll want to make a proper script with better usability out of it? I'm fine with manually hacking on an example, but a proper CLI would be neat.

@8-bit-fox
Copy link
Author

8-bit-fox commented Oct 8, 2020

Hi @piegamesde

I am using KPXC 2.6.1 from the Ubuntu PPA.

Thanks for your link, that's very convenient 🙂

Well, I am currently running the script in a background task. If I unlock, it can retrieve the credentials, if not, I see the pop-up but error messages will be somewhere in the background.
However, I would like to provide it also to third-parties. There, we already use pykeepass, but this tool would be much more convenient. AFAIU, @hrehfeld would like this module to be a pure library. Should we provide the CLI via an extra repository?

P.S.: Do you know if its possible to retrieve keys by credential path? (E.g., accounts/amazon)

@piegamesde
Copy link
Contributor

Well, I am currently running the script in a background task. If I unlock, it can retrieve the credentials, if not, I see the pop-up but error messages will be somewhere in the background.

This is not specific enough to make sense of what's going on. Does it work when the database is already unlocked? Does it fail even if you click "Accept" on the pop-up or only if you deny? If you're running a background task, isn't it consequent if the error message is somewhere in the background?

Should we provide the CLI via an extra repository?

I'm strongly in favour of this. But if you want to reuse the code from the PR, maybe best discuss that there.

P.S.: Do you know if its possible to retrieve keys by credential path? (E.g., accounts/amazon)

Pretty sure it's not. But I designed the feature in an extensible way, it shouldn't be hard to add a keepassxc://by-path/ URI matcher. (If you implement this, ping me in the pull request.)

@8-bit-fox
Copy link
Author

This is not specific enough to make sense of what's going on. Does it work when the database is already unlocked? Does it fail even if you click "Accept" on the pop-up or only if you deny? If you're running a background task, isn't it consequent if the error message is somewhere in the background?

Yes, it works, when the DB is unlocked. It does not fail, if the DB is unlocked and I accept. Actually, I just wanted to agree with you that it currently does not bother me too much, as it is not a show-stopper for me.

I'm strongly in favour of this. But if you want to reuse the code from the PR, maybe best discuss that there.

Well, my current version is much simpler. I think about it.

Pretty sure it's not. But I designed the feature in an extensible way, it shouldn't be hard to add a keepassxc://by-path/ URI matcher. (If you implement this, ping me in the pull request.)
I'll have a look.

@yan12125
Copy link
Contributor

When there are multiple clients connecting via the kepassxc-browser protocol, a race condition may result in failure of assert response_nonce == expected_nonce. Since keepassxreboot/keepassxc#7404, the race condition is gone, and I don't observe assertion failures anymore.

By the way, there is another bug in wait_for_unlock(), and the fix is simple - #30

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

No branches or pull requests

4 participants