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

Use new Secure Message API in RubyThemis #402

Merged
merged 3 commits into from Mar 4, 2019

Conversation

ilammy
Copy link
Collaborator

@ilammy ilammy commented Mar 1, 2019

It's easy to use the new API even if I don't know Ruby, just "attach" the new functions instead of the old ones and use them.

Just like with other language wrappers, improve key validation in Secure Message: perform early checks of private and public key and signal the user if there's anything wrong with them.

It's easy to use the new API even if I don't know Ruby, just "attach"
the new functions instead of the old ones and use them.
Just like with other language wrappers, improve key validation in
Secure Message: perform early checks of private and public key and
signal the user if there's anything wrong with them.
@ilammy ilammy added the W-RbThemis ♦️ Wrapper: RbThemis, Ruby API, Ruby gems label Mar 1, 2019
@ilammy ilammy requested a review from shadinua March 1, 2019 23:13
@ilammy
Copy link
Collaborator Author

ilammy commented Mar 1, 2019

@shadinua I'd like you to take a look at this since I don't actually know Ruby, I just kept hitting the keyboard until the tests passed.

For example, I don't really understand why the 'module methods' have to be defined that way for it to work and why the constants from ThemisImports are not visible when the module seems to be 'imported' and the functions are visible. I'm also concerned about visibility of the new key checking methods, they might have been unintentionally exported for the users.

raise ThemisError, "Secure Message: invalid public key"
end
if not Themis.private_key(private_key)
raise ThemisError, "Secure Message: public key used instead of private"
Copy link
Contributor

Choose a reason for hiding this comment

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

nice checks :)

@@ -360,24 +410,31 @@ def Ssign(*args)
deprecate :Ssign, :s_sign, 2018, 6

def s_sign(private_key, message)
if not valid_key(private_key)
Copy link
Contributor

Choose a reason for hiding this comment

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

technically speaking for user both these checks are the same -- wrong / empty / bad key.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather prefer to merge these checks in one, or use "Secure Message: private key used instead of public" as second error message

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a difference between "using an incorrect key" and "using a key incorrectly". If we get to the second check we are sure that the key is at least valid so we should not tell the user that their key is garbled or something.

I guess I'll reuse the same messages as for the encrypt/decrypt mode.

@ilammy ilammy merged commit d0e70c1 into cossacklabs:master Mar 4, 2019
@ilammy ilammy deleted the smessage-api-ruby branch March 28, 2019 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
W-RbThemis ♦️ Wrapper: RbThemis, Ruby API, Ruby gems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants