Implementation of basic API authentication #1228
Conversation
Will help protect exposure of API port to the Web
ethminer/main.cpp
Outdated
| @@ -817,6 +822,7 @@ class MinerCLI | |||
|
|
|||
| #if API_CORE | |||
| int m_api_port = 0; | |||
| string m_api_password = ""; | |||
libapicore/ApiServer.cpp
Outdated
| @@ -36,7 +37,7 @@ void ApiServer::start() | |||
| return; | |||
| } | |||
|
|
|||
| cnote << "Api server listening for connections on port " + to_string(m_acceptor.local_endpoint().port()); | |||
| cnote << "Api server listening for connections on port " + to_string(m_acceptor.local_endpoint().port()) << (m_password.empty() ? "" : " Authentication needed"); | |||
There was a problem hiding this comment.
Maybe add a . before Authentication needed?
There was a problem hiding this comment.
It's just a sophistication ... that line will be quickly scrolled outside the boundaries of the screen.
Anyway ... ok.
There was a problem hiding this comment.
You can remove "for connections" part of the message.
libapicore/ApiServer.cpp
Outdated
| @@ -142,63 +143,101 @@ void ApiConnection::processRequest(Json::Value& requestObject) | |||
| std::string _method = requestObject.get("method", "").asString(); | |||
| jRes["id"] = requestObject.get("id", 0).asInt(); | |||
|
|
|||
| // Check authentication | |||
| if (!m_is_authenticated) { | |||
There was a problem hiding this comment.
Have you tried integrate clang-format to your editor. It would make it easier to have consistent coding style.
There was a problem hiding this comment.
Ok for formatting.
libapicore/ApiServer.cpp
Outdated
| else { | ||
| Json::Value jPrm = requestObject["params"]; | ||
| if (!jPrm.isMember("password") || jPrm["password"].empty() || !jPrm["password"].isString()) { | ||
| jRes["error"]["code"] = -32602; |
There was a problem hiding this comment.
The error code -32602 is only for "Invalid params". We should assign our own error code here.
There was a problem hiding this comment.
And in fact here we have invalid params (the "password" member is missing).
Maybe you refer to validation of password ?
libapicore/ApiServer.cpp
Outdated
| jRes["result"] = true; | ||
| m_farm.shuffle(); | ||
| if (!requestObject.isMember("params") || requestObject["params"].empty() || !requestObject["params"].isObject()) { | ||
| jRes["error"]["code"] = -32602; |
There was a problem hiding this comment.
This should be -32600 error code "Invalid Request". http://www.jsonrpc.org/specification#error_object
libapicore/ApiServer.cpp
Outdated
|
|
||
| // Replies back to (check for liveness) | ||
| jRes["result"] = "pong"; | ||
| jRes["error"]["code"] = -32601; |
libapicore/ApiServer.cpp
Outdated
| m_is_authenticated = true; | ||
| } | ||
| else { | ||
| jRes["error"]["code"] = -32602; |
| Farm& m_farm; | ||
|
|
||
| bool m_is_authenticated = true; |
There was a problem hiding this comment.
Can we always init that to false but require authentication only if the password is not empty?
There was a problem hiding this comment.
And in fact it is like that. It's m_is_authenticated, not m_requires_auth
|
I know - Claymore is not the standard. |
|
@StefanOberhumer you're driving me crazy with that Claymore's ... :) Accomplished. |
|
@StefanOberhumer does rubbishmore implement also a specific method name ? |
|
@StefanOberhumer |
libapicore/ApiServer.cpp
Outdated
| } | ||
| else | ||
| { | ||
| jRes["error"]["code"] = -32001; |
There was a problem hiding this comment.
The error codes from and including -32768 to -32000 are reserved
This can be any value, e.g.1.
There was a problem hiding this comment.
Will use HTTP like range
-401 Unauthorized
libapicore/ApiServer.cpp
Outdated
| } | ||
| else | ||
| { | ||
| jRes["error"]["code"] = -32601; |
There was a problem hiding this comment.
Will use HTTP like range
-403 Forbidden
Will help protect exposure of API port to the Web