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

Eliminate white space around ristretto key when reading key file #188

Closed
wants to merge 2 commits into from

Conversation

hacklschorsch
Copy link
Member

It is easy to erroneously add extra white space to a text file. For example when writing with an editor like nano or when using echo without the -n argument.

Handing that white space over to python-challenge-bypass-ristretto's decode_base64() method will make it fail in a rather opaque way, see LeastAuthority/python-challenge-bypass-ristretto#37.

This eliminates white space including newlines around the key expected to be in that file.

It is easy to erroneously add extra white space to a text file. For example when writing with an editor like `nano` or when using `echo` without the `-n` argument.

Handing that white space over to python-challenge-bypass-ristretto's `decode_base64()` method will make it fail in a rather opaque way, see LeastAuthority/python-challenge-bypass-ristretto#37.

This eliminates white space including newlines around the key expected to be in that file.
Copy link
Collaborator

@exarkun exarkun left a comment

Choose a reason for hiding this comment

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

Thanks. Do you feel up to writing a unit test, too?

@hacklschorsch
Copy link
Member Author

I tried for one hour to run the existing tests, then gave up.

@exarkun
Copy link
Collaborator

exarkun commented May 13, 2021

Yea, fair :( I also started looking at the state of the test suite and it's not great. I'll loop back here after it has been revivified...

@hacklschorsch
Copy link
Member Author

I could also make the point that Unit testing here might not be the right thing:

  • the SigningKey.decode_base64() unit is imported from another library and tested there. It specifies the parameter it expects, and it's our job to adhere to the spec
  • the FilePath(...).getContent() unit is from the standard library and widely tested,
  • as is the strip() unit.

It's the integration of all this we want to test.
Testing that integration makes good sense, and I also think it is a good location to spec our expected input.

I think it is worth while to add an integration test for this.

But honestly, trying to be very street smart I want to do something else.

... on purpose so devs know what to expect from users like me.
@hacklschorsch
Copy link
Member Author

That violates the principle that Unit tests should test one thing only. But OTOH, if some other component has a problem with a signing key like that, it should be exposed as well.

@exarkun
Copy link
Collaborator

exarkun commented May 17, 2021

In #198 I took a shot at the kind of "unit test" I think provides value here. I started w/ this PR for that branch so I'm gonna say that PR supersedes this one.

@exarkun exarkun closed this May 17, 2021
@hacklschorsch hacklschorsch deleted the patch-1 branch May 17, 2021 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants