Skip to content

Commit

Permalink
MB-51962: Add support for internal server cert
Browse files Browse the repository at this point in the history
Map email addresses internal@internal.couchbase.com to
a user named @internal. This user does not have access
to any buckets, and no privileges. Its sole purpose
is to allow the internal components to connect to
memcached over TLS when the encryption mode is set
to mandatory. They would then have to authenticate
to memcached by sing SASL.

Change-Id: Icd521f60c9ffc303bd1b45d7a23db7a6b29351d5
Reviewed-on: https://review.couchbase.org/c/kv_engine/+/174348
Tested-by: Trond Norbye <trond.norbye@couchbase.com>
Reviewed-by: Jim Walker <jim@couchbase.com>
  • Loading branch information
trondn committed May 9, 2022
1 parent a9f0765 commit b0a2bcf
Show file tree
Hide file tree
Showing 8 changed files with 152 additions and 3 deletions.
44 changes: 41 additions & 3 deletions daemon/connection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "connection.h"

#include "buckets.h"
#include "client_cert_config.h"
#include "connections.h"
#include "cookie.h"
#include "external_auth_manager_thread.h"
Expand Down Expand Up @@ -1200,7 +1201,44 @@ void Connection::ssl_read_callback(bufferevent* bev, void* ctx) {
SSL_VERIFY_FAIL_IF_NO_PEER_CERT);
// Check certificate
if (cert) {
auto [status, name] = Settings::instance().lookupUser(cert.get());
class ServerAuthMapper {
public:
static std::pair<cb::x509::Status, std::string> lookup(
X509* cert) {
static ServerAuthMapper inst;
return inst.mapper->lookupUser(cert);
}

protected:
ServerAuthMapper() {
mapper = cb::x509::ClientCertConfig::create(R"({
"prefixes": [
{
"path": "san.uri",
"prefix": "email:",
"delimiter": "",
"suffix":"@internal.couchbase.com"
}
]
})"_json);
}

std::unique_ptr<cb::x509::ClientCertConfig> mapper;
};

auto [status, name] = ServerAuthMapper::lookup(cert.get());
if (status == cb::x509::Status::Success) {
if (name == "internal") {
name = "@internal";
} else {
status = cb::x509::Status::NoMatch;
}
} else {
auto pair = Settings::instance().lookupUser(cert.get());
status = pair.first;
name = std::move(pair.second);
}

switch (status) {
case cb::x509::Status::NoMatch:
audit_auth_failure(instance,
Expand Down Expand Up @@ -1324,9 +1362,9 @@ bool Connection::tryAuthFromSslCert(const std::string& userName,
getPeername(),
cipherName,
cb::UserDataView(user.name));
// Connections authenticated by using X.509 certificates should not
// External users authenticated by using X.509 certificates should not
// be able to use SASL to change it's identity.
saslAuthEnabled = false;
saslAuthEnabled = internal;
} catch (const cb::rbac::NoSuchUserException& e) {
setAuthenticated(false);
LOG_WARNING("{}: User [{}] is not defined as a user in Couchbase",
Expand Down
23 changes: 23 additions & 0 deletions rbac/privilege_database.cc
Original file line number Diff line number Diff line change
Expand Up @@ -391,10 +391,33 @@ PrivilegeDatabase::PrivilegeDatabase(const nlohmann::json& json, Domain domain)
const std::string username = it.key();
userdb.emplace(username, UserEntry(username, it.value(), domain));
}

// The internal user should _not_ have access to buckets or any privileges
// it is only being used to connect to the server when mandatory mode
// is enabled. Each component should then perform SASL AUTH to connect
// to the actual user. By explicitly removing all access we won't allow
// anyone to sneak in a configuration change and suddenly stop
// performing the authentication.
auto iter = userdb.find("@internal");
if (iter != userdb.end()) {
userdb.erase(iter);
}

if (domain == Domain::Local) {
static const nlohmann::json internal =
R"({"buckets":{},"privileges":[],"domain":"local"})"_json;
userdb.emplace("@internal",
UserEntry("@internal", internal, Domain::Local));
}
}

std::unique_ptr<PrivilegeDatabase> PrivilegeDatabase::updateUser(
const std::string& user, Domain domain, UserEntry& entry) const {
if (user == "@internal") {
throw std::invalid_argument(
"PrivilegeDatabase::updateUser: The @internal user cannot be "
"changed");
}
// Check if they differ
auto iter = userdb.find(user);
if (iter != userdb.end() && entry == iter->second) {
Expand Down
13 changes: 13 additions & 0 deletions tests/cert/clients/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ Generate_kv_engine_X509_Certificate(NAME jane
EXTFILE ca.ext
CA_TARGET Generate_kv_engine_X509_Test_Intermediate_Certificate)

Generate_kv_engine_X509_Certificate(NAME internal
CONFIG internal.config
ROOT ../root/ca_root
EXTFILE internal-ca.ext
CA_TARGET Generate_kv_engine_X509_Test_Root_Certificate)

add_custom_command(OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/jane_chain.pem
COMMAND
${CMAKE_COMMAND}
Expand All @@ -41,6 +47,8 @@ add_custom_target(Generate_kv_engine_X509_Test_Client_Certificate
${CMAKE_CURRENT_BINARY_DIR}/john.cert
${CMAKE_CURRENT_BINARY_DIR}/trond.cert
${CMAKE_CURRENT_BINARY_DIR}/trond.key
${CMAKE_CURRENT_BINARY_DIR}/internal.cert
${CMAKE_CURRENT_BINARY_DIR}/internal.key
${CMAKE_CURRENT_BINARY_DIR}/jane_chain.pem)

add_custom_target(Dump_kv_engine_X509_Test_Client_Certificate
Expand All @@ -59,6 +67,11 @@ add_custom_target(Dump_kv_engine_X509_Test_Client_Certificate
x509
-in ${CMAKE_CURRENT_BINARY_DIR}/trond.cert
-text
COMMAND
${OPENSSL_BINARY}
x509
-in ${CMAKE_CURRENT_BINARY_DIR}/internal.cert
-text
DEPENDS
Generate_kv_engine_X509_Test_Client_Certificate
)
9 changes: 9 additions & 0 deletions tests/cert/clients/internal-ca.ext
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
subjectKeyIdentifier = hash
authorityKeyIdentifier = keyid:always,issuer:always
basicConstraints = CA:false
extendedKeyUsage = clientAuth
keyUsage = digitalSignature
subjectAltName = @alt_names

[ alt_names ]
URI.1 = email:internal@internal.couchbase.com
12 changes: 12 additions & 0 deletions tests/cert/clients/internal.config
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
[req]
default_bits = 2048
prompt = no
default_md = sha256
encrypt_key = no
distinguished_name = dn

[ dn ]
C=NO
O=Couchbase Inc
OU=kv engine
CN=internal system user
15 changes: 15 additions & 0 deletions tests/testapp/rbac.json
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,21 @@
],
"domain": "local"
},
"@fts": {
"buckets": {
"default" : [
"Read",
"Insert",
"Delete",
"Upsert",
"SystemXattrRead",
"SystemXattrWrite"
]
},
"privileges": [
],
"domain": "local"
},
"bucket-1": {
"buckets": {
"bucket-1": [
Expand Down
38 changes: 38 additions & 0 deletions tests/testapp/testapp_cert_tests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -357,3 +357,41 @@ TEST_F(SslCertTest, MB50564_intermediate_cert_in_trusted_store) {
connection->connect();
connection->setXerrorSupport(true);
}

TEST_F(SslCertTest, RecognizeInternalUserFromCert) {
reconfigure_client_cert_auth("mandatory", "san.uri", "couchbase://", "@");

auto connection = createConnection();
ASSERT_TRUE(connection) << "Failed to locate a SSL port";
setClientCertData(*connection, "internal");
connection->connect();
connection->setXerrorSupport(true);
nlohmann::json json;
connection->stats(
[&json](const auto&, const auto& value) {
json = nlohmann::json::parse(value);
},
"connections self");
EXPECT_FALSE(json.empty());
EXPECT_TRUE(json["internal"]);
EXPECT_EQ("@internal", json["user"]["name"]) << json["user"].dump(2);

// the internal user should not be allowed to select any buckets
auto rsp = connection->execute(BinprotGenericCommand(
cb::mcbp::ClientOpcode::SelectBucket, "default"));
EXPECT_EQ(cb::mcbp::Status::Eaccess, rsp.getStatus());

// We should be allowed to change user
connection->authenticate("@fts", mcd_env->getPassword("@fts"));
connection->stats(
[&json](const auto&, const auto& value) {
json = nlohmann::json::parse(value);
},
"connections self");
EXPECT_FALSE(json.empty());
EXPECT_TRUE(json["internal"]);
EXPECT_EQ("@fts", json["user"]["name"]) << json["user"].dump(2);
rsp = connection->execute(BinprotGenericCommand(
cb::mcbp::ClientOpcode::SelectBucket, "default"));
EXPECT_TRUE(rsp.isSuccess());
}
1 change: 1 addition & 0 deletions tests/testapp/testapp_environment.cc
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,7 @@ class McdEnvironmentImpl : public McdEnvironment {

const std::unordered_map<std::string, std::string> users{
{"@admin", "password"},
{"@fts", "<#w`?D4QwY/x%j8M"},
{"bucket-1", "1S|=,%#x1"},
{"bucket-2", "secret"},
{"bucket-3", "1S|=,%#x1"},
Expand Down

0 comments on commit b0a2bcf

Please sign in to comment.