use "application.statusKey" instead of "application.secret" …

[asolntsev]

…for /@status requests

[theo]

Assuming we're trying to introduce a different key for the purposes of getting status, should we change the flag from --secret to --status-key to avoid confusion?

[asolntsev]

Hm... Yes, probably. Good point.

…

On May 22, 2017 4:35 PM, "Theodore Nguyen-Cao" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In documentation/commands/cmd-status.txt <#1144 (comment)>: > @@ -25,8 +25,8 @@ ~ ~ --secret: Assuming we're trying to introduce a different key for the purposes of getting status, should we change the flag from --secret to --status-key to avoid confusion? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#1144 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARE3bCaCvJTKCGjtnRXLo2L9tcksU3sks5r8Y8ggaJpZM4NiSgT> .

[asolntsev]

Thank you @theo ! I renamed --status to --status-key.
Additionally, I extracted "/@status" and "/@Kill" part of CorePlugin to a dedicated class PlayStatusPlugin (single responsibility principle). And renamed CorePlugin to EnhancerPlugin because now it's the only job of this plugin.

@xael-fry Please also review the changes.

[asolntsev]

@xael-fry @flybyray Please review this pull request. I would like to merge it.

[xael-fry]

Merged thanks

[sbeigel]

How ist this supposed to work? Why did you remove the signing part in the plugin (-> server side)? I thought your goal was to not re-use the app secret but a distinct key. But I think we need to keep the signing to check the message "@status" signed w/ the "new" statusKey?!

[xael-fry]

@asolntsev I let you answer this question

[asolntsev]

@sbeigel @xael-fry Sorry for the long replay.
We removed the signing part because it was a potential security problem. It allowed hacker to bruteforce secretKey/statusKey.

[sbeigel]

But the play status command signs the string "@status" with the application.statusKey property. Then, on the server-side, this signed message is compared to the application.statusKey property. How should this work? This will never be equal :)
To make this work right now, you have to sign the message "@status" by yourself, set this signed message as statusKey in the app.conf and pass your signing key as an explicit argument to the play status command... The default case in play status is useless (as it doesn't work).

[asolntsev]

Sorry, I don't know what means "the default case". We use the following command to get application status: curl --header "authorization: YOUR-SECRET-WORD" http://localhost:9000/@status Andrei Solntsev 2017-11-28 11:13 GMT+02:00 Sebastian Beigel <notifications@github.com>:

…

But the play status command signs the string ***@***.***" with the application.statusKey property. Then, on the server-side, this signed message is compared to the application.statusKey property. How should this work? This will never be equal :) To make this work right now, you have to sign the message ***@***.***" by yourself, set this signed message as statusKey in the app.conf and pass *your* signing key as an explicit argument to the play status command... The default case in play status is useless (as it doesn't work). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1144 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARE3ZF6CCzegZrP1lRfzN4JeaVY7FxMks5s687QgaJpZM4NiSgT> .

[sbeigel]

Yes, that's what I thought :)

But this change effectively renders the play status command useless, as this command (look at the python code) signs the string "@status", either with the given secret or (that is the default) with the app.conf's property statusKey.

I understand that you want a "classic" API/secret key style. But then the play status command has to be converted to this approach as well, i.e. remove the singing part and take the passed argument or the app.conf's statusKey as auth message.

[asolntsev]

Ok, you mean "play status" command? I never used it. Yes, sure: if it became broken, it should be fixed. Anyway, I hope you also agree that it's better to break some insecure feature than to keep it working. :) Andrei Solntsev 2017-11-28 11:25 GMT+02:00 Sebastian Beigel <notifications@github.com>:

…

Yes, that's what I thought :) But this change effectively renders the play status command useless, as this command (look at the python code) signs the string ***@***.***", either with the given secret or (that is the default) with the app.conf's property statusKey. I understand that you want a "classic" API/secret key style. But then the play status command has to be converted to this approach as well, i.e. remove the singing part and take the passed argument *or* the app.conf's statusKey as auth message. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1144 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARE3YDcv32iIB7id5ar0cLiy-kEzHOKks5s69GIgaJpZM4NiSgT> .

[sbeigel]

Yes, I do :)

I can fix the status command (in the way just described) and make a PR if you want...

[asolntsev]

Sure, it would be great. Thank you! Andrei Solntsev 2017-11-28 11:38 GMT+02:00 Sebastian Beigel <notifications@github.com>:

…

Yes, I do :) I can fix the status command (in the way just described) and make a PR if you want... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1144 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARE3SyOKH_cGV-lZaFxN-NWerxl3y3kks5s69SagaJpZM4NiSgT> .

[sbeigel]

see #1209