Skip to content
This repository has been archived by the owner on Apr 24, 2022. It is now read-only.

Implementation of basic API authentication #1228

Merged
merged 7 commits into from
Jun 6, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

## Unreleased

### Added

- Basic API authentication to protect exposure of API port to the internet [#1228](https://github.com/ethereum-mining/ethminer/pull/1228).

## 0.15.0rc1

Expand Down
8 changes: 7 additions & 1 deletion ethminer/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,11 @@ class MinerCLI
->group(APIGroup)
->check(CLI::Range(-65535, 65535));

app.add_option("--api-password", m_api_password,
"Set the password to protect interaction with Api server. If not set any connection is granted access."
"Be advised passwords are sent unencrypted over plain tcp !!")
->group(APIGroup);

app.add_option("--http-port", m_http_port,
"Set the web api port, the miner should listen to. Use 0 to disable. Data shown depends on hwmon setting", true)
->group(APIGroup)
Expand Down Expand Up @@ -710,7 +715,7 @@ class MinerCLI

#if API_CORE

ApiServer api(m_io_service, abs(m_api_port), (m_api_port < 0) ? true : false, f);
ApiServer api(m_io_service, abs(m_api_port), (m_api_port < 0) ? true : false, m_api_password, f);
api.start();

http_server.run(m_http_port, &f, m_show_hwmonitors, m_show_power);
Expand Down Expand Up @@ -817,6 +822,7 @@ class MinerCLI

#if API_CORE
int m_api_port = 0;
string m_api_password = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

The = "" is not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok

unsigned m_http_port = 0;
#endif

Expand Down
131 changes: 85 additions & 46 deletions libapicore/ApiServer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@

#include <ethminer-buildinfo.h>

ApiServer::ApiServer(boost::asio::io_service& io_service, int portnum, bool readonly, Farm& f) :
ApiServer::ApiServer(boost::asio::io_service& io_service, int portnum, bool readonly, string password, Farm& f) :
m_readonly(readonly),
m_password(std::move(password)),
m_portnumber(portnum),
m_acceptor(io_service),
m_io_strand(io_service),
Expand All @@ -17,7 +18,7 @@ void ApiServer::start()
if (m_portnumber == 0) return;

m_running.store(true, std::memory_order_relaxed);

tcp::endpoint endpoint(tcp::v4(), m_portnumber);

// Try to bind to port number
Expand All @@ -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");
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a . before Authentication needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just a sophistication ... that line will be quickly scrolled outside the boundaries of the screen.
Anyway ... ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove "for connections" part of the message.

m_workThread = std::thread{ boost::bind(&ApiServer::begin_accept, this) };

}
Expand All @@ -60,7 +61,7 @@ void ApiServer::begin_accept()
if (!isRunning()) return;

dev::setThreadName("Api");
std::shared_ptr<ApiConnection> session = std::make_shared<ApiConnection>(m_acceptor.get_io_service(), ++lastSessionId, m_readonly, m_farm);
std::shared_ptr<ApiConnection> session = std::make_shared<ApiConnection>(m_acceptor.get_io_service(), ++lastSessionId, m_readonly, m_password, m_farm);
m_acceptor.async_accept(session->socket(), m_io_strand.wrap(boost::bind(&ApiServer::handle_accept, this, session, boost::asio::placeholders::error)));
}

Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried integrate clang-format to your editor. It would make it easier to have consistent coding style.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok for formatting.

if (_method == "api_authorize") {

if (_method == "miner_getstat1")
{
jRes["result"] = getMinerStat1();
}
else if (_method == "miner_getstathr")
{
jRes["result"] = getMinerStatHR();
}
else if (_method == "miner_shuffle")
{

// Gives nonce scrambler a new range
cnote << "Miner Shuffle requested";
jRes["result"] = true;
m_farm.shuffle();
if (!requestObject.isMember("params") || requestObject["params"].empty() || !requestObject["params"].isObject()) {
jRes["error"]["code"] = -32602;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be -32600 error code "Invalid Request". http://www.jsonrpc.org/specification#error_object

jRes["error"]["message"] = "Missing params object member";
}
else {
Json::Value jPrm = requestObject["params"];
if (!jPrm.isMember("password") || jPrm["password"].empty() || !jPrm["password"].isString()) {
jRes["error"]["code"] = -32602;
Copy link
Contributor

Choose a reason for hiding this comment

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

The error code -32602 is only for "Invalid params". We should assign our own error code here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And in fact here we have invalid params (the "password" member is missing).
Maybe you refer to validation of password ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right. Sorry.

jRes["error"]["message"] = "Missing password";
}
else
{
if (jPrm.get("password", "").asString() == m_password) {
m_is_authenticated = true;
}
else {
jRes["error"]["code"] = -32602;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong error code.

jRes["error"]["message"] = "Invalid password";
}
}
}

}
else if (_method == "miner_ping")
{
}
else {

// Replies back to (check for liveness)
jRes["result"] = "pong";
jRes["error"]["code"] = -32601;
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong error code.

jRes["error"]["message"] = "Authorization needed";

}
}
else if (_method == "miner_restart")

if (m_is_authenticated)
{
// Send response to client of success
// and invoke an async restart
// to prevent locking
if (m_readonly)
if (_method == "miner_getstat1")
{
jRes["error"]["code"] = -32601;
jRes["error"]["message"] = "Method not available";
jRes["result"] = getMinerStat1();
}
else
else if (_method == "miner_getstathr")
{
cnote << "Miner Restart requested";
jRes["result"] = getMinerStatHR();
}
else if (_method == "miner_shuffle")
{

// Gives nonce scrambler a new range
cnote << "Miner Shuffle requested";
jRes["result"] = true;
m_farm.restart_async();
m_farm.shuffle();

}
else if (_method == "miner_ping")
{

}
else if (_method == "miner_reboot")
{
// Replies back to (check for liveness)
jRes["result"] = "pong";

// Not implemented yet
jRes["error"]["code"] = -32601;
jRes["error"]["message"] = "Method not implemented";
}
else if (_method == "miner_restart")
{
// Send response to client of success
// and invoke an async restart
// to prevent locking
if (m_readonly)
{
jRes["error"]["code"] = -32601;
jRes["error"]["message"] = "Method not available";
}
else
{
cnote << "Miner Restart requested";
jRes["result"] = true;
m_farm.restart_async();
}

}
else
{
}
else if (_method == "miner_reboot")
{

// Not implemented yet
jRes["error"]["code"] = -32601;
jRes["error"]["message"] = "Method not implemented";

}
else
{

// Any other method not found
jRes["error"]["code"] = -32601;
jRes["error"]["message"] = "Method not found";
}

// Any other method not found
jRes["error"]["code"] = -32601;
jRes["error"]["message"] = "Method not found";
}

// Send response
Expand Down
16 changes: 13 additions & 3 deletions libapicore/ApiServer.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,18 @@ class ApiConnection
{
public:

ApiConnection(boost::asio::io_service& io_service, int id, bool readonly, Farm& f) :
ApiConnection(boost::asio::io_service& io_service, int id, bool readonly, string password, Farm& f) :
m_sessionId(id),
m_socket(io_service),
m_io_strand(io_service),
m_readonly(readonly),
m_farm(f) {}
m_password(std::move(password)),
m_farm(f)
{

if (!m_password.empty()) m_is_authenticated = false;

}

~ApiConnection() {}

Expand Down Expand Up @@ -58,16 +64,19 @@ class ApiConnection
Json::FastWriter m_jWriter;

bool m_readonly = false ;
std::string m_password = "";
Farm& m_farm;

bool m_is_authenticated = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we always init that to false but require authentication only if the password is not empty?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And in fact it is like that. It's m_is_authenticated, not m_requires_auth


};


class ApiServer
{
public:

ApiServer(boost::asio::io_service& io_service, int portnum, bool readonly, Farm& f);
ApiServer(boost::asio::io_service& io_service, int portnum, bool readonly, string password, Farm& f);
bool isRunning() { return m_running.load(std::memory_order_relaxed); };
void start();
void stop();
Expand All @@ -81,6 +90,7 @@ class ApiServer

std::thread m_workThread;
std::atomic<bool> m_readonly = { false };
std::string m_password = "";
std::atomic<bool> m_running = { false };
int m_portnumber;
tcp::acceptor m_acceptor;
Expand Down