BackupStoreContext: simplify authentication of local connections

Instead of going through contortions to fake an account number that
BackupProtocolLogin::DoCommand will accept, we check the SSL client certificate
in BackupStoreContext::DoCommandHook instead, and only when a remote protocol
is used (bbstored), not at all for local protocols (S3).

lib/backupstore/BackupCommands.cpp

@@ -137,24 +137,6 @@ std::auto_ptr<BackupProtocolMessage> BackupProtocolLogin::DoCommand(BackupProtoc
 {
 	CHECK_PHASE(Phase_Login)
 
-	// Check given client ID against the ID in the certificate certificate
-	// and that the client actually has an account on this machine
-	if(mClientID != rContext.GetClientID())
-	{
-		BOX_WARNING("Failed login from client ID " <<
-			BOX_FORMAT_ACCOUNT(mClientID) << ": "
-			"wrong certificate for this account");
-		return PROTOCOL_ERROR(Err_BadLogin);
-	}
-
-	if(!rContext.GetClientHasAccount())
-	{
-		BOX_WARNING("Failed login from client ID " <<
-			BOX_FORMAT_ACCOUNT(mClientID) << ": "
-			"no such account on this server");
-		return PROTOCOL_ERROR(Err_BadLogin);
-	}
-
 	// If we need to write, check that nothing else has got a write lock
 	if((mFlags & Flags_ReadOnly) != Flags_ReadOnly)
 	{

lib/backupstore/BackupStoreContext.cpp

@@ -52,6 +52,7 @@
 BackupStoreContext::BackupStoreContext(int32_t ClientID,
 	HousekeepingInterface* pHousekeeping, const std::string& rConnectionDetails)
 : mConnectionDetails(rConnectionDetails),
+  mCheckClientID(true), // in this case we DO want to check the ClientID
   mClientID(ClientID),
   mpHousekeeping(pHousekeeping),
   mProtocolPhase(Phase_START),
@@ -74,10 +75,13 @@ BackupStoreContext::BackupStoreContext(int32_t ClientID,
 //		Created: 2015/11/02
 //
 // --------------------------------------------------------------------------
-BackupStoreContext::BackupStoreContext(BackupFileSystem& rFileSystem, int32_t ClientID,
+BackupStoreContext::BackupStoreContext(BackupFileSystem& rFileSystem,
 	HousekeepingInterface* pHousekeeping, const std::string& rConnectionDetails)
 : mConnectionDetails(rConnectionDetails),
-  mClientID(ClientID),
+  mCheckClientID(false), // in this case we DO NOT want to check the ClientID
+  // mClientID is only used by SetClientHasAccount(...) which cannot be used with supplied
+  // rFileSystem, so set it to a dummy value here.
+  mClientID(0),
   mpHousekeeping(pHousekeeping),
   mProtocolPhase(Phase_START),
   mClientHasAccount(false),
@@ -112,6 +116,39 @@ BackupStoreContext::~BackupStoreContext()
 	}
 }
 
+std::auto_ptr<BackupProtocolMessage> BackupStoreContext::DoCommandHook(
+	BackupProtocolMessage& command, BackupProtocolReplyable& protocol,
+	IOStream* data_stream)
+{
+	if(command.GetType() == BackupProtocolLogin::TypeID)
+	{
+		BackupProtocolLogin& login_command = dynamic_cast<BackupProtocolLogin &>(command);
+		// BackupProtocolLogin::DoCommand will check the phase later.
+
+		// Iff we have been initialised with a ClientID (which comes from the common name in
+		// the client's SSL certificate) then check that it matches the ID that the client
+		// is trying to login as.
+		if(mCheckClientID && login_command.GetClientID() != mClientID)
+		{
+			BOX_WARNING("Failed login from client ID " <<
+				BOX_FORMAT_ACCOUNT(login_command.GetClientID()) << ": "
+				"wrong certificate for this account: " <<
+				BOX_FORMAT_ACCOUNT(mClientID));
+			return PROTOCOL_ERROR(Err_BadLogin);
+		}
+
+		if(!mClientHasAccount)
+		{
+			BOX_WARNING("Failed login from client ID " <<
+				BOX_FORMAT_ACCOUNT(mClientID) << ": "
+				"no such account on this server");
+			return PROTOCOL_ERROR(Err_BadLogin);
+		}
+	}
+
+	// Return no message to call BackupProtocolLogin::DoCommand() normally:
+	return std::auto_ptr<BackupProtocolMessage>();
+}
 
 void BackupStoreContext::FlushDirectoryCache()
 {
@@ -1668,7 +1705,8 @@ std::string BackupStoreContext::GetAccountIdentifier()
 	{
 		// This can happen if this BackupStoreContext was constructed without a
 		// BackupFileSystem, in which case mClientID is the account number from the SSL
-		// certificate.
+		// certificate, and mCheckClientID is true.
+		ASSERT(mCheckClientID);
 		std::ostringstream oss;
 		oss << "unknown (" << BOX_FORMAT_ACCOUNT(mClientID) << ")";
 		return oss.str();

lib/backupstore/BackupStoreContext.h

@@ -49,7 +49,7 @@ class BackupStoreContext
 	BackupStoreContext(int32_t ClientID,
 		HousekeepingInterface* mpHousekeeping,
 		const std::string& rConnectionDetails);
-	BackupStoreContext(BackupFileSystem& rFileSystem, int32_t ClientID,
+	BackupStoreContext(BackupFileSystem& rFileSystem,
 		HousekeepingInterface* mpHousekeeping,
 		const std::string& rConnectionDetails);
 	virtual ~BackupStoreContext();
@@ -59,7 +59,6 @@ class BackupStoreContext
 
 public:
 	void CleanUp();
-	int32_t GetClientID() {return mClientID;}
 
 	typedef enum
 	{
@@ -99,18 +98,15 @@ class BackupStoreContext
 	}
 
 	// TODO: stop using this version, which has the side-effect of creating a
-	// BackupStoreFileSystem:
+	// BackupStoreFileSystem. But BackupStoreDaemon currently uses it, because it creates a
+	// BackupStoreContext even for client connections with no valid account.
 	void SetClientHasAccount(const std::string &rStoreRoot, int StoreDiscSet);
 	void SetClientHasAccount()
 	{
 		mClientHasAccount = true;
 	}
-	bool GetClientHasAccount() const {return mClientHasAccount;}
 	std::auto_ptr<BackupProtocolMessage> DoCommandHook(BackupProtocolMessage& command,
-		BackupProtocolReplyable& protocol, IOStream* data_stream = NULL)
-	{
-		return std::auto_ptr<BackupProtocolMessage>();
-	}
+		BackupProtocolReplyable& protocol, IOStream* data_stream = NULL);
 
 	// Store info
 	void LoadStoreInfo();
@@ -200,6 +196,11 @@ class BackupStoreContext
 	std::auto_ptr<IOStream> GetBlockIndexReconstructed(int64_t ObjectID, int64_t InDirectory);
 
 	// Info
+	// GetClientID() is only used by BackupProtocolLogin::DoCommand to check that the supplied
+	// ID matches the one in the certificate, which BackupStoreDaemon supplies when it
+	// constructs the BackupStoreContext. Thus it is only used, and only initialised, when
+	// constructed by the non-BackupFileSystem constructor, which only BackupStoreDaemon
+	// should use.
 	int32_t GetClientID() const {return mClientID;}
 	const std::string& GetConnectionDetails() { return mConnectionDetails; }
 	std::string GetAccountIdentifier();
@@ -221,6 +222,7 @@ class BackupStoreContext
 	std::vector<int64_t> GetPatchChain(int64_t ObjectID, int64_t InDirectory);
 
 	std::string mConnectionDetails;
+	bool mCheckClientID;
 	int32_t mClientID;
 	HousekeepingInterface *mpHousekeeping;
 	ProtocolPhase mProtocolPhase;

lib/backupstore/StoreTestUtils.h

@@ -67,8 +67,7 @@ void set_refcount(int64_t ObjectID, uint32_t RefCount = 1);
 void init_context(TLSContext& rContext);
 
 #define CREATE_LOCAL_CONTEXT_AND_PROTOCOL(filesystem, context_name, protocol_name, read_only) \
-	BackupStoreContext context_name(filesystem, 0x01234567, NULL, \
-		"fake test connection"); \
+	BackupStoreContext context_name(filesystem, NULL, "fake test connection"); \
 	BackupProtocolLocal2 protocol_name(context_name, 0x01234567, read_only);
 
 //! Opens a connection to the server (bbstored).

test/backupstore/testbackupstore.cpp

@@ -2345,8 +2345,7 @@ bool test_cannot_open_multiple_writable_connections(RaidAndS3TestSpecs::Speciali
 #ifndef BOX_LOCK_TYPE_F_SETLK
 	BOX_TRACE("Opening read-write local protocol (expected to fail to get a write lock)");
 	{
-		BackupStoreContext context_2(fs_2, 0x01234567, NULL,
-			"fake test connection");
+		BackupStoreContext context_2(fs_2, NULL, "fake test connection");
 		context_2.SetClientHasAccount();
 		BackupProtocolLocal protocol_writable_2(context_2); // !ReadOnly
 		protocol_writable_2.QueryVersion(BACKUP_STORE_SERVER_VERSION);
@@ -2808,7 +2807,7 @@ bool test_login_with_disabled_account(RaidAndS3TestSpecs::Specialisation& spec)
 	// BLOCK
 	{
 		// Open a local (virtual) connection to the server
-		BackupStoreContext context(fs, 0x01234567, NULL, "fake test connection");
+		BackupStoreContext context(fs, NULL, "fake test connection");
 		BackupProtocolLocal2 protocol(context, 0x01234567, false, // !read_only
 			false); // !login
 
@@ -2960,9 +2959,9 @@ bool test_account_limits_respected(RaidAndS3TestSpecs::Specialisation& spec)
 	SETUP_TEST_SPECIALISED(spec);
 	BackupFileSystem& fs(spec.control().GetFileSystem());
 
-	BackupStoreContext rwContext(fs, 0x01234567, NULL, // mpHousekeeping
+	BackupStoreContext rwContext(fs, NULL, // mpHousekeeping
 		"fake test connection"); // rConnectionDetails
-	BackupStoreContext roContext(fs, 0x01234567, NULL, // mpHousekeeping
+	BackupStoreContext roContext(fs, NULL, // mpHousekeeping
 		"fake test connection"); // rConnectionDetails
 
 	// Set a really small hard limit