10000 Fixing Cluster Authentication (#3313) · MohammedDeveloper/arangodb@5bc0be1 · GitHub
[go: up one dir, main page]
Skip to content

Commit 5bc0be1

Browse files
graetzerfceller
graetzer
authored and
fceller
committed
Fixing Cluster Authentication (arangodb#3313)
* Adding a missing loadFromDB call * Various bootstrap fixes * Fixed revoke error
1 parent 0ab5531 commit 5bc0be1

File tree

7 files changed

+79
-65
lines changed

7 files changed

+79
-65
lines changed

‎arangod/Aql/ExecutionPlan.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,7 @@ static std::unique_ptr<graph::BaseOptions> CreateTraversalOptions(
121121
arangodb::traverser::TraverserOptions::UniquenessLevel::GLOBAL;
122122
}
123123
} else if (name == "uniqueEdges" && value->isStringValue()) {
124+
// path is the default
124125
if (value->stringEquals("none", true)) {
125126
options->uniqueEdges =
126127
arangodb::traverser::TraverserOptions::UniquenessLevel::NONE;

‎arangod/Graph/Traverser.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -149,8 +149,6 @@ Traverser::Traverser(arangodb::traverser::TraverserOptions* opts,
149149
}
150150
}
151151

152-
Traverser::~Traverser() {}
153-
154152
bool arangodb::traverser::Traverser::edgeMatchesConditions(VPackSlice e,
155153
StringRef vid,
156154
uint64_t depth,

‎arangod/Graph/Traverser.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ class Traverser {
185185
/// @brief Destructor
186186
//////////////////////////////////////////////////////////////////////////////
187187

188-
virtual ~Traverser();
188+
virtual ~Traverser() {};
189189

190190
void done() { _done = true; }
191191

‎arangod/RestServer/BootstrapFeature.cpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include "Aql/QueryList.h"
2727
#include "Cluster/ClusterInfo.h"
2828
#include "Cluster/ServerState.h"
29+
#include "GeneralServer/AuthenticationFeature.h"
2930
#include "GeneralServer/RestHandlerFactory.h"
3031
#include "Logger/Logger.h"
3132
#include "ProgramOptions/Parameters.h"
@@ -131,6 +132,9 @@ static void raceForClusterBootstrap() {
131132
sleep(1);
132133
continue;
133134
}
135+
136+
LOG_TOPIC(DEBUG, Logger::STARTUP) << "Creating the root user";
137+
AuthenticationFeature::INSTANCE->authInfo()->createRootUser();
134138

135139
LOG_TOPIC(DEBUG, Logger::STARTUP)
136140
<< "raceForClusterBootstrap: bootstrap done";
@@ -157,6 +161,11 @@ void BootstrapFeature::start() {
157161
if (!ss->isRunningInCluster()) {
158162
LOG_TOPIC(DEBUG, Logger::STARTUP) << "Running server/server.js";
159163
V8DealerFeature::DEALER->loadJavaScriptFileInAllContexts(vocbase, "server/server.js", nullptr);
164+
// Agency is not allowed to call this
165+
if (ServerState::instance()->isSingleServer()) {
166+
// only creates root user if it does not exist
167+
AuthenticationFeature::INSTANCE->authInfo()->createRootUser();
168+
}
160169
} else if (ss->isCoordinator()) {
161170
LOG_TOPIC(DEBUG, Logger::STARTUP) << "Racing for cluster bootstrap...";
162171
raceForClusterBootstrap();

‎arangod/RestServer/UpgradeFeature.cpp

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -107,19 +107,19 @@ void UpgradeFeature::start() {
107107
if (_upgradeCheck) {
108108
upgradeDatabase();
109109

110-
if (!init->restoreAdmin() && !init->defaultPassword().empty()) {
110+
if (!init->restoreAdmin() && !init->defaultPassword().empty() &&
111+
ServerState::instance()->isSingleServerOrCoordinator()) {
111112
ai->updateUser("root", [&](AuthUserEntry& entry) {
112113
entry.updatePassword(init->defaultPassword());
113114
});
114115
}
115116
}
116117

117118
// change admin user
118-
if (init->restoreAdmin()) {
119-
Result res;
120-
121-
res = ai->removeAllUsers();
122-
119+
if (init->restoreAdmin() &&
120+
ServerState::instance()->isSingleServerOrCoordinator()) {
121+
122+
Result res = ai->removeAllUsers();
123123
if (res.fail()) {
124124
LOG_TOPIC(WARN, arangodb::Logger::FIXME) << "failed to create clear users: "
125125
<< res.errorMessage();
@@ -128,7 +128,6 @@ void UpgradeFeature::start() {
128128
}
129129

130130
res = ai->storeUser(true, "root", init->defaultPassword(), true);
131-
132131
if (res.fail() && res.errorNumber() == TRI_ERROR_USER_NOT_FOUND) {
133132
res = ai->storeUser(false, "root", init->defaultPassword(), true);
134133
}
@@ -139,7 +138,6 @@ void UpgradeFeature::start() {
139138
*_result = EXIT_FAILURE;
140139
return;
141140
}
142-
143141
*_result = EXIT_SUCCESS;
144142
}
145143

‎arangod/VocBase/AuthInfo.cpp

Lines changed: 57 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,9 @@ std::string AuthInfo::jwtSecret() {
9090
// private, must be called with _authInfoLock in write mode
9191
bool AuthInfo::parseUsers(VPackSlice const& slice) {
9292
TRI_ASSERT(slice.isArray());
93+
TRI_ASSERT(_authInfo.empty());
94+
TRI_ASSERT(_authBasicCache.empty());
9395

94-
_authInfo.clear();
95-
_authBasicCache.clear();
9696
for (VPackSlice const& authSlice : VPackArrayIterator(slice)) {
9797
VPackSlice s = authSlice.resolveExternal();
9898

@@ -108,10 +108,7 @@ bool AuthInfo::parseUsers(VPackSlice const& slice) {
108108
// we also need to insert inactive users into the cache here
109109
// otherwise all following update/replace/remove operations on the
110110
// user will fail
111-
112-
// intentional copy, as we'll be moving out of auth soon
113-
std::string username = auth.username();
114-
_authInfo.emplace(std::move(username), std::move(auth));
111+
_authInfo.emplace(auth.username(), std::move(auth));
115112
}
116113

117114
return true;
@@ -124,7 +121,7 @@ static std::shared_ptr<VPackBuilder> QueryAllUsers(
124121
LOG_TOPIC(DEBUG, arangodb::Logger::FIXME) << "system database is unknown";
125122
THROW_ARANGO_EXCEPTION(TRI_ERROR_INTERNAL);
126123
}
127-
124+
128125
// we cannot set this execution context, otherwise the transaction
129126
// will ask us again for permissions and we get a deadlock
130127
ExecContext* oldExe = ExecContext::CURRENT;
@@ -146,7 +143,9 @@ static std::shared_ptr<VPackBuilder> QueryAllUsers(
146143
(queryResult.code == TRI_ERROR_QUERY_KILLED)) {
147144
THROW_ARANGO_EXCEPTION(TRI_ERROR_REQUEST_CANCELED);
148145
}
149-
return std::shared_ptr<VPackBuilder>();
146+
THROW_ARANGO_EXCEPTION_MESSAGE(queryResult.code,
147+
"Error executing user query");
148+
//return std::shared_ptr<VPackBuilder>();
150149
}
151150

152151
VPackSlice usersSlice = queryResult.result->slice();
@@ -173,7 +172,7 @@ static VPackBuilder QueryUser(aql::QueryRegistry* queryRegistry,
173172
ExecContext* oldExe = ExecContext::CURRENT;
174173
ExecContext::CURRENT = nullptr;
175174
TRI_DEFER(ExecContext::CURRENT = oldExe);
176-
175+
177176
std::string const queryStr("FOR u IN _users FILTER u.user == @name RETURN u");
178177
auto emptyBuilder = std::make_shared<VPackBuilder>();
179178

@@ -192,7 +191,8 @@ static VPackBuilder QueryUser(aql::QueryRegistry* queryRegistry,
192191
(queryResult.code == TRI_ERROR_QUERY_KILLED)) {
193192
THROW_ARANGO_EXCEPTION(TRI_ERROR_REQUEST_CANCELED);
194193
}
195-
THROW_ARANGO_EXCEPTION_MESSAGE(queryResult.code, "query error");
194+
THROW_ARANGO_EXCEPTION_MESSAGE(queryResult.code,
195+
"Error executing user query");
196196
}
197197

198198
VPackSlice usersSlice = queryResult.result->slice();
@@ -225,6 +225,14 @@ static void ConvertLegacyFormat(VPackSlice doc, VPackBuilder& result) {
225225
// private, will acquire _authInfoLock in write-mode and release it.
226226
// will also aquire _loadFromDBLock and release it
227227
void AuthInfo::loadFromDB() {
228+
TRI_ASSERT(_queryRegistry != nullptr);
229+
TRI_ASSERT(ServerState::instance()->isSingleServerOrCoordinator());
230+
auto role = ServerState::instance()->getRole();
231+
if (role != ServerState::ROLE_SINGLE &&
232+
role != ServerState::ROLE_COORDINATOR) {
233+
_outdated = false;
234+
return;
235+
}
228236
if (!_outdated) {
229237
return;
230238
}
@@ -236,46 +244,46 @@ void AuthInfo::loadFromDB() {
236244
return;
237245
}
238246

239-
auto role = ServerState::instance()->getRole();
240-
241-
if (role != ServerState::ROLE_SINGLE &&
242-
role != ServerState::ROLE_COORDINATOR) {
243-
_outdated = false;
244-
return;
245-
}
246-
247-
TRI_ASSERT(_queryRegistry != nullptr);
248-
std::shared_ptr<VPackBuilder> builder = QueryAllUsers(_queryRegistry);
249-
250247
WRITE_LOCKER(writeLocker, _authInfoLock);
251-
_authBasicCache.clear();
252-
253-
if (builder) {
254-
VPackSlice usersSlice = builder->slice();
255-
if (usersSlice.length() != 0) {
256-
parseUsers(usersSlice);
248+
try {
249+
std::shared_ptr<VPackBuilder> builder = QueryAllUsers(_queryRegistry);
250+
_authInfo.clear();
251+
_authBasicCache.clear();
252+
if (builder) {
253+
VPackSlice usersSlice = builder->slice();
254+
if (usersSlice.length() != 0) {
255+
parseUsers(usersSlice);
256+
}
257257
}
258+
_outdated = _authInfo.empty() == true;
259+
} catch (...) {
260+
LOG_TOPIC(WARN, Logger::AUTHENTICATION)
261+
<< "Exception when loading users from db";
262+
_outdated = true;
258263
}
259-
260-
if (_authInfo.empty()) {
261-
insertInitial();
262-
}
263-
_outdated = false;
264264
}
265265

266-
// private, must be called with _authInfoLock in write mode
267-
void AuthInfo::insertInitial() {
268-
if (!_authInfo.empty()) {
266+
// only call from the boostrap feature, must be sure to be the only one
267+
void AuthInfo::createRootUser() {
268+
loadFromDB();
269+
270+
MUTEX_LOCKER(locker, _loadFromDBLock);
271+
WRITE_LOCKER(writeLocker, _authInfoLock);
272+
auto it = _authInfo.find("root");
273+
if (it != _authInfo.end()) {
274+
LOG_TOPIC(TRACE, Logger::AUTHENTICATION) << "Root already exists";
269275
return;
270276
}
271-
277+
TRI_ASSERT(_authInfo.empty());
278+
272279
try {
273280
// Attention:
274281
// the root user needs to have a specific rights grant
275282
// to the "_system" database, otherwise things break
276283
auto initDatabaseFeature =
277284
application_features::ApplicationServer::getFeature<
278285
InitDatabaseFeature>("InitDatabase");
286+
TRI_ASSERT(initDatabaseFeature != nullptr);
279287

280288
AuthUserEntry entry = AuthUserEntry::newUser(
281289
"root", initDatabaseFeature->defaultPassword(), AuthSource::COLLECTION);
@@ -300,15 +308,14 @@ Result AuthInfo::storeUserInternal(AuthUserEntry const& entry, bool replace) {
300308
if (vocbase == nullptr) {
301309
return Result(TRI_ERROR_INTERNAL);
302310
}
303-
311+
304312
// we cannot set this execution context, otherwise the transaction
305313
// will ask us again for permissions and we get a deadlock
306314
ExecContext* oldExe = ExecContext::CURRENT;
307315
ExecContext::CURRENT = nullptr;
308316
TRI_DEFER(ExecContext::CURRENT = oldExe);
309317

310-
std::shared_ptr<transaction::Context> ctx(
311-
new transaction::StandaloneContext(vocbase));
318+
auto ctx = transaction::StandaloneContext::Create(vocbase);
312319
SingleCollectionTransaction trx(ctx, TRI_COL_NAME_USERS,
313320
AccessMode::Type::WRITE);
314321
trx.addHint(transaction::Hints::Hint::SINGLE_OPERATION);
@@ -443,15 +450,14 @@ static Result UpdateUser(VPackSlice const& user) {
443450
if (vocbase == nullptr) {
444451
return Result(TRI_ERROR_INTERNAL);
445452
}
446-
453+
447454
// we cannot set this execution context, otherwise the transaction
448455
// will ask us again for permissions and we get a deadlock
449456
ExecContext* oldExe = ExecContext::CURRENT;
450457
ExecContext::CURRENT = nullptr;
451458
TRI_DEFER(ExecContext::CURRENT = oldExe);
452459

453-
std::shared_ptr<transaction::Context> ctx(
454-
new transaction::StandaloneContext(vocbase));
460+
auto ctx = transaction::StandaloneContext::Create(vocbase);
455461
SingleCollectionTransaction trx(ctx, TRI_COL_NAME_USERS,
456462
AccessMode::Type::WRITE);
457463
trx.addHint(transaction::Hints::Hint::SINGLE_OPERATION);
@@ -480,6 +486,8 @@ Result AuthInfo::enumerateUsers(
480486
return r;
481487
}
482488
}
489+
// must also clear the basic cache here because the secret may be
490+
// invalid now if the password was changed
483491
_authBasicCache.clear();
484492
}
485493
// we need to reload data after the next callback
@@ -494,7 +502,6 @@ Result AuthInfo::updateUser(std::string const& user,
494502
}
495503
loadFromDB();
496504
Result r;
497-
VPackBuilder data;
498505
{ // we require an consisten view on the user object
499506
WRITE_LOCKER(guard, _authInfoLock);
500507
auto it = _authInfo.find(user);
@@ -503,7 +510,7 @@ Result AuthInfo::updateUser(std::string const& user,
503510
}
504511
TRI_ASSERT(!it->second.key().empty());
505512
func(it->second);
506-
data = it->second.toVPackBuilder();
513+
VPackBuilder data = it->second.toVPackBuilder();
507514
r = UpdateUser(data.slice());
508515
// must also clear the basic cache here because the secret may be
509516
// invalid now if the password was changed
@@ -529,8 +536,8 @@ Result AuthInfo::accessUser(
529536
}
530537

531538
VPackBuilder AuthInfo::serializeUser(std::string const& user) {
532-
// loadFromDB();
533-
// READ_LOCKER(guard, _authInfoLock)
539+
loadFromDB();
540+
// will query db directly, no need for _authInfoLock
534541
VPackBuilder doc = QueryUser(_queryRegistry, user);
535542
VPackBuilder result;
536543
if (!doc.isEmpty()) {
@@ -545,15 +552,14 @@ static Result RemoveUserInternal(AuthUserEntry const& entry) {
545552
if (vocbase == nullptr) {
546553
return Result(TRI_ERROR_INTERNAL);
547554
}
548-
555+
549556
// we cannot set this execution context, otherwise the transaction
550557
// will ask us again for permissions and we get a deadlock
551558
ExecContext* oldExe = ExecContext::CURRENT;
552559
ExecContext::CURRENT = nullptr;
553560
TRI_DEFER(ExecContext::CURRENT = oldExe);
554561

555-
std::shared_ptr<transaction::Context> ctx(
556-
new transaction::StandaloneContext(vocbase));
562+
auto ctx = transaction::StandaloneContext::Create(vocbase);
557563
SingleCollectionTransaction trx(ctx, TRI_COL_NAME_USERS,
558564
AccessMode::Type::WRITE);
559565

@@ -648,7 +654,6 @@ Result AuthInfo::setConfigData(std::string const& user,
648654
partial.close();
649655

650656
return UpdateUser(partial.slice());
651-
;
652657
}
653658

654659
VPackBuilder AuthInfo::getUserData(std::string const& username) {
@@ -754,6 +759,8 @@ AuthLevel AuthInfo::canUseCollection(std::string const& username,
754759
}
755760

756761
// public called from HttpCommTask.cpp and VstCommTask.cpp
762+
// should only lock if required, otherwise we will serialize all
763+
// requests whether we need to or not
757764
AuthResult AuthInfo::checkAuthentication(AuthenticationMethod authType,
758765
std::string const& secret) {
759766
switch (authType) {

‎arangod/VocBase/AuthInfo.h

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ class AuthResult {
5050
: _username(username), _authorized(false) {}
5151

5252
std::string _username;
53+
/// User exists and password was checked
5354
bool _authorized;
5455
};
5556

@@ -78,6 +79,10 @@ class AuthInfo {
7879

7980
/// Trigger eventual reload, user facing API call
8081
void reloadAllUsers();
82+
83+
/// Create the root user with a default password, will fail if the user
84+
/// already exists. Only ever call if you can guarantee to be in charge
85+
void createRootUser();
8186

8287
VPackBuilder allUsers();
8388
/// Add user from arangodb, do not use for LDAP users
@@ -113,14 +118,10 @@ class AuthInfo {
113118
std::string jwtSecret();
114119
std::string generateJwt(VPackBuilder const&);
115120
std::string generateRawJwt(VPackBuilder const&);
116-
/*
117-
std::shared_ptr<AuthContext> getAuthContext(std::string const& username,
118-
std::string const& database);*/
119121

120122
private:
121123
void loadFromDB();
122124
bool parseUsers(velocypack::Slice const& slice);
123-
void insertInitial();
124125
Result storeUserInternal(AuthUserEntry const& user, bool replace);
125126

126127
AuthResult checkAuthenticationBasic(std::string const& secret);

0 commit comments

Comments
 (0)
0