LGTM On Wed, Oct 1, 2014 at 12:39 PM, Helga Velroyen <[email protected]> wrote:
> > > On Tue, Sep 30, 2014 at 2:12 PM, Petr Pudlak <[email protected]> wrote: > >> On Tue, Sep 30, 2014 at 09:58:59AM +0200, Helga Velroyen wrote: >> >>> Hi Petr, >>> >>> after our offline discussion last week, I checked this patch again and it >>> seems it was a bug to remove the "or from_authorized_keys" line. I >>> double-checked and ran another QA. >>> >>> Besides that It turns out I had decided to always leave the node's own >>> key >>> in its own 'authorized_key" file (as it solves some problems and it does >>> not affect the security boundary since if you can access the machine >>> already as root, it does not really make it worse if you can ssh to it >>> again.) >>> >>> However, I realized that with that the code was unnecessarily complicated >>> and I was able to shorten it. Here is the interdiff: >>> >>> diff --git a/lib/backend.py b/lib/backend.py >>> index c2bdde7..1afec42 100644 >>> --- a/lib/backend.py >>> +++ b/lib/backend.py >>> @@ -1598,8 +1598,7 @@ def RemoveNodeSshKey(node_uuid, node_name, >>> from_authorized_keys, >>> True, True, False, False, False, >>> ssh_port, base_data, ssconf_store) >>> >>> - authorized_keys_to_clear = {} >>> - if clear_authorized_keys or from_public_keys: >>> + if clear_authorized_keys or from_public_keys or from_authorized_keys: >>> >> >> Since now checking `from_authorized_keys` is removed below and it's never >> used in this whole big `if` block, should it be added here or not? IIUC, >> `data` is cleared at the beginning and if only `from_authorized_keys` is >> set and not the other two flags, the final call to `run_cmd_fn` just gets >> empty `data`, right? > > > Right. Now that I think about it, it does not make sense to keep the "or > from_authorized_keys". I ran QA again and verified that. Sorry for the > confusion, it is just very long ago that I wrote this code in the first > place. > > Here is an interdiff (to the original patch, ignore the first interdiff): > > diff --git a/lib/backend.py b/lib/backend.py > index c2bdde7..797bea1 100644 > --- a/lib/backend.py > +++ b/lib/backend.py > @@ -1598,7 +1598,6 @@ def RemoveNodeSshKey(node_uuid, node_name, > from_authorized_keys, > True, True, False, False, False, > ssh_port, base_data, ssconf_store) > > - authorized_keys_to_clear = {} > if clear_authorized_keys or from_public_keys: > data = {} > _InitSshUpdateData(data, noded_cert_file, ssconf_store) > @@ -1608,18 +1607,13 @@ def RemoveNodeSshKey(node_uuid, node_name, > from_authorized_keys, > raise errors.OpExecError("No SSH port information available for" > " node '%s', which is leaving the > cluster.") > > - authorized_keys_to_clear = {} > if clear_authorized_keys: > other_master_candidate_uuids = [uuid for uuid in > master_candidate_uuids > if uuid != node_uuid] > candidate_keys = ssh.QueryPubKeyFile(other_master_candidate_uuids, > key_file=pub_key_file) > - authorized_keys_to_clear = candidate_keys > - if from_authorized_keys: > - authorized_keys_to_clear[node_uuid] = keys[node_uuid] > - if authorized_keys_to_clear: > data[constants.SSHS_SSH_AUTHORIZED_KEYS] = \ > - (constants.SSHS_REMOVE, authorized_keys_to_clear) > + (constants.SSHS_REMOVE, candidate_keys) > > if from_public_keys: > data[constants.SSHS_SSH_PUBLIC_KEYS] = \ > > > Cheers, > Helga > > > > > > > >> >> >> data = {} >>> _InitSshUpdateData(data, noded_cert_file, ssconf_store) >>> cluster_name = data[constants.SSHS_CLUSTER_NAME] >>> @@ -1608,18 +1607,13 @@ def RemoveNodeSshKey(node_uuid, node_name, >>> from_authorized_keys, >>> raise errors.OpExecError("No SSH port information available for" >>> " node '%s', which is leaving the >>> cluster.") >>> >>> - authorized_keys_to_clear = {} >>> if clear_authorized_keys: >>> other_master_candidate_uuids = [uuid for uuid in >>> master_candidate_uuids >>> if uuid != node_uuid] >>> candidate_keys = ssh.QueryPubKeyFile(other_master_candidate_uuids, >>> >> >> diff --git a/lib/backend.py b/lib/backend.py >> >> index c2bdde7..797bea1 100644 >> >> --- a/lib/backend.py >> >> +++ b/lib/backend.py >> >> @@ -1598,7 +1598,6 @@ def RemoveNodeSshKey(node_uuid, node_name, >>> from_authorized_keys, >> >> True, True, False, False, False, >> >> ssh_port, base_data, ssconf_store) >> >> >> >> - authorized_keys_to_clear = {} >> >> if clear_authorized_keys or from_public_keys: >> >> data = {} >> >> _InitSshUpdateData(data, noded_cert_file, ssconf_store) >> >> @@ -1608,18 +1607,13 @@ def RemoveNodeSshKey(node_uuid, node_name, >>> from_authorized_keys, >> >> raise errors.OpExecError("No SSH port information available for" >> >> " node '%s', which is leaving the >>> cluster.") >> >> >> >> - authorized_keys_to_clear = {} >> >> if clear_authorized_keys: >> >> other_master_candidate_uuids = [uuid for uuid in >>> master_candidate_uuids >> >> if uuid != node_uuid] >> >> candidate_keys = ssh.QueryPubKeyFile(other_master_candidate_uuids, >> >> key_file=pub_key_file) >> >> - authorized_keys_to_clear = candidate_keys >> >> - if from_authorized_keys: >> >> - authorized_keys_to_clear[node_uuid] = keys[node_uuid] >> >> - if authorized_keys_to_clear: >> >> data[constants.SSHS_SSH_AUTHORIZED_KEYS] = \ >> >> - (constants.SSHS_REMOVE, authorized_keys_to_clear) >> >> + (constants.SSHS_REMOVE, candidate_keys) >> >> >> >> if from_public_keys: >> >> data[constants.SSHS_SSH_PUBLIC_KEYS] = \ >> >> key_file=pub_key_file) >>> - authorized_keys_to_clear = candidate_keys >>> - if from_authorized_keys: >>> - authorized_keys_to_clear[node_uuid] = keys[node_uuid] >>> - if authorized_keys_to_clear: >>> data[constants.SSHS_SSH_AUTHORIZED_KEYS] = \ >>> - (constants.SSHS_REMOVE, authorized_keys_to_clear) >>> + (constants.SSHS_REMOVE, candidate_keys) >>> >>> if from_public_keys: >>> data[constants.SSHS_SSH_PUBLIC_KEYS] = \ >>> >>> >>> Cheers, >>> Helga >>> >>> On Tue, Sep 2, 2014 at 4:19 PM, Helga Velroyen <[email protected]> >>> wrote: >>> >>> This patch implements the removal of a node's SSH key >>>> from all nodes' "authorized_keys" files when it is >>>> demoted from being master candidate to being a normal >>>> node. It also adds the adding of a node's SSH key >>>> when it is promoted from normal node to master >>>> candidate. >>>> >>>> Signed-off-by: Helga Velroyen <[email protected]> >>>> --- >>>> lib/backend.py | 27 ++++++++++++++++----------- >>>> lib/cmdlib/node.py | 4 +--- >>>> lib/config.py | 20 ++++++++++++++++++++ >>>> 3 files changed, 37 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/lib/backend.py b/lib/backend.py >>>> index 717298c..efdc00a 100644 >>>> --- a/lib/backend.py >>>> +++ b/lib/backend.py >>>> @@ -1436,16 +1436,22 @@ def AddNodeSshKey(node_uuid, node_name, >>>> >>>> # Check and fix sanity of key file >>>> keys_by_name = ssh.QueryPubKeyFile([node_name], >>>> key_file=pub_key_file) >>>> - keys_by_uuid = {} >>>> - if not keys_by_name or node_name not in keys_by_name: >>>> - raise errors.SshUpdateError("No keys found for the new node '%s' in >>>> the" >>>> - " list of public SSH keys." % >>>> node_name) >>>> + keys_by_uuid = ssh.QueryPubKeyFile([node_uuid], >>>> key_file=pub_key_file) >>>> + >>>> + if (not keys_by_name or node_name not in keys_by_name) \ >>>> + and (not keys_by_uuid or node_uuid not in keys_by_uuid): >>>> + raise errors.SshUpdateError( >>>> + "No keys found for the new node '%s' (UUID %s) in the list of >>>> public" >>>> + " SSH keys, neither for the name or the UUID" % (node_name, >>>> node_uuid)) >>>> else: >>>> - # Replace the name by UUID in the file as the name should only be >>>> used >>>> - # temporarily >>>> - ssh.ReplaceNameByUuid(node_uuid, node_name, >>>> error_fn=errors.SshUpdateError, >>>> - key_file=pub_key_file) >>>> - keys_by_uuid[node_uuid] = keys_by_name[node_name] >>>> + if node_name in keys_by_name: >>>> + keys_by_uuid = {} >>>> + # Replace the name by UUID in the file as the name should only be >>>> used >>>> + # temporarily >>>> + ssh.ReplaceNameByUuid(node_uuid, node_name, >>>> + error_fn=errors.SshUpdateError, >>>> + key_file=pub_key_file) >>>> + keys_by_uuid[node_uuid] = keys_by_name[node_name] >>>> >>>> # Update the master node's key files >>>> if to_authorized_keys: >>>> @@ -1584,7 +1590,7 @@ def RemoveNodeSshKey(node_uuid, node_name, >>>> from_authorized_keys, >>>> ssh_port, base_data, ssconf_store) >>>> >>>> authorized_keys_to_clear = {} >>>> - if clear_authorized_keys or from_public_keys or from_authorized_keys: >>>> + if clear_authorized_keys or from_public_keys: >>>> data = {} >>>> _InitSshUpdateData(data, noded_cert_file, ssconf_store) >>>> cluster_name = data[constants.SSHS_CLUSTER_NAME] >>>> @@ -1595,7 +1601,6 @@ def RemoveNodeSshKey(node_uuid, node_name, >>>> from_authorized_keys, >>>> >>>> authorized_keys_to_clear = {} >>>> if clear_authorized_keys: >>>> - # We never clear a node's key from its own 'authorized_keys' file >>>> other_master_candidate_uuids = [uuid for uuid in >>>> master_candidate_uuids >>>> if uuid != node_uuid] >>>> candidate_keys = ssh.QueryPubKeyFile(other_ >>>> master_candidate_uuids, >>>> diff --git a/lib/cmdlib/node.py b/lib/cmdlib/node.py >>>> index 823ebb6..1fc6f06 100644 >>>> --- a/lib/cmdlib/node.py >>>> +++ b/lib/cmdlib/node.py >>>> @@ -1563,9 +1563,7 @@ class LUNodeRemove(LogicalUnit): >>>> potential_master_candidate = \ >>>> self.op.node_name in potential_master_candidates >>>> ssh_port_map = GetSshPortMap(potential_master_candidates, >>>> self.cfg) >>>> - master_candidate_uuids = [uuid for (uuid, node_info) >>>> - in self.cfg.GetAllNodesInfo().items() >>>> - if node_info.master_candidate] >>>> + master_candidate_uuids = self.cfg.GetMasterCandidateUuids() >>>> master_node = self.cfg.GetMasterNode() >>>> result = self.rpc.call_node_ssh_key_remove( >>>> [master_node], >>>> diff --git a/lib/config.py b/lib/config.py >>>> index 6be2814..e2b2bd5 100644 >>>> --- a/lib/config.py >>>> +++ b/lib/config.py >>>> @@ -2576,6 +2576,26 @@ class ConfigWriter(object): >>>> return frozenset(self._UnlockedGetNodeInfo(uuid).group >>>> for uuid in node_uuids) >>>> >>>> + def _UnlockedGetMasterCandidateUuids(self): >>>> + """Get the list of UUIDs of master candidates. >>>> + >>>> + @rtype: list of strings >>>> + @return: list of UUIDs of all master candidates. >>>> + >>>> + """ >>>> + return [node.uuid for node in self._ConfigData().nodes.values() >>>> + if node.master_candidate] >>>> + >>>> + @_ConfigSync(shared=1) >>>> + def GetMasterCandidateUuids(self): >>>> + """Get the list of UUIDs of master candidates. >>>> + >>>> + @rtype: list of strings >>>> + @return: list of UUIDs of all master candidates. >>>> + >>>> + """ >>>> + return self._UnlockedGetMasterCandidateUuids() >>>> + >>>> def _UnlockedGetMasterCandidateStats(self, exceptions=None): >>>> """Get the number of current and maximum desired and possible >>>> candidates. >>>> >>>> -- >>>> 2.1.0.rc2.206.gedb03e5 >>>> >>>> >>>> >>> >>> -- >>> Helga Velroyen | Software Engineer | [email protected] | >>> >>> Google Germany GmbH >>> Dienerstr. 12 >>> 80331 München >>> >>> Registergericht und -nummer: Hamburg, HRB 86891 >>> Sitz der Gesellschaft: Hamburg >>> Geschäftsführer: Graham Law, Christine Elizabeth Flores >>> >> > > > -- > Helga Velroyen | Software Engineer | [email protected] | > > Google Germany GmbH > Dienerstr. 12 > 80331 München > > Registergericht und -nummer: Hamburg, HRB 86891 > Sitz der Gesellschaft: Hamburg > Geschäftsführer: Graham Law, Christine Elizabeth Flores >
