Windows password auto deletion #606

Merged
merged 16 commits into from Jun 10, 2015

Projects

None yet

5 participants

@triggers
Contributor
triggers commented Jun 5, 2015

No description provided.

@axsh-bot axsh-bot commented on an outdated diff Jun 5, 2015
dcmgr/lib/dcmgr/node_modules/maintenance.rb
@@ -32,6 +34,15 @@ def expire_ip_handles
}
end
+ def automatic_windows_password_deletion
+ # logger.debug "Checking for windows passwords to delete"
+
+ Dcmgr::Models::Instance.alives.expired.each { |handle|
+ logger.debug "Automatically deleting encrypted password for instance with uuid:#{handle.canonical_uuid}"
+ handle.set({ :encrypted_password => nil, :password_will_be_deleted_at => nil }).save_changes
+ }
+ end
+
@axsh-bot
axsh-bot Jun 5, 2015 Member

[rails_best_practices]

  • remove trailing whitespace
@axsh-bot axsh-bot commented on an outdated diff Jun 5, 2015
dcmgr/lib/dcmgr/configurations/dcmgr.rb
end
+ class Windows < Fuguta::Configuration
+ param :delete_password_on_request, default: false
+ end
+
+ on_initialize_hook do
+ @config[:windows] = Windows.new(self)
+ end
+
@axsh-bot
axsh-bot Jun 5, 2015 Member

[rails_best_practices]

  • remove trailing whitespace
@axsh-bot axsh-bot commented on an outdated diff Jun 5, 2015
dcmgr/lib/dcmgr/endpoints/12.03/instances.rb
@@ -186,8 +186,15 @@ def set_monitoring_parameters(instance)
get '/:instance_id/password' do
@instance = find_by_uuid(:Instance, params[:instance_id])
- respond_with(R::InstancePassword.new(@instance).generate)
- end
+ theresponse = R::InstancePassword.new(@instance).generate
+
@axsh-bot
axsh-bot Jun 5, 2015 Member

[rails_best_practices]

  • remove trailing whitespace
@triggers triggers commented on an outdated diff Jun 5, 2015
dcmgr/lib/dcmgr/models/instance.rb
@@ -37,6 +37,7 @@ class Instance < AccountResource
subset(:runnings, {:state => STATE_RUNNING})
subset(:running_or_initializing, {:state => [STATE_RUNNING, STATE_INITIALIZING]})
subset(:stops, {:state => STATE_STOPPED})
+ subset(:expired) { (password_will_be_deleted_at > 0) && (password_will_be_deleted_at <= Time.now) }
@triggers
triggers Jun 5, 2015 Contributor

This line seems to work when I test it, but I'm not 100% sure if this is OK, because password_will_be_deleted_at can be nil. Can some expert on Sequel comment on this?

triggers added some commits Jun 5, 2015
@triggers triggers Fixed trailing whitespace 27a6b07
@triggers triggers Fixed two more trailing whitespace text
b9e611c
@axsh-bot axsh-bot commented on an outdated diff Jun 5, 2015
dcmgr/lib/dcmgr/node_modules/maintenance.rb
@@ -32,6 +34,15 @@ def expire_ip_handles
}
end
+ def automatic_windows_password_deletion
+ # logger.debug "Checking for windows passwords to delete"
+
+ Dcmgr::Models::Instance.alives.expired.each { |handle|
+ logger.debug "Automatically deleting encrypted password for instance with uuid:#{handle.canonical_uuid}"
+ handle.set({ :encrypted_password => nil, :password_will_be_deleted_at => nil }).save_changes
+ }
+ end
+
@axsh-bot
axsh-bot Jun 5, 2015 Member

[rails_best_practices]

  • remove trailing whitespace
@axsh-bot axsh-bot commented on an outdated diff Jun 5, 2015
dcmgr/lib/dcmgr/endpoints/12.03/instances.rb
@@ -186,8 +186,15 @@ def set_monitoring_parameters(instance)
get '/:instance_id/password' do
@instance = find_by_uuid(:Instance, params[:instance_id])
- respond_with(R::InstancePassword.new(@instance).generate)
- end
+ theresponse = R::InstancePassword.new(@instance).generate
+
@axsh-bot
axsh-bot Jun 5, 2015 Member

[rails_best_practices]

  • remove trailing whitespace
@axsh-bot
Member
axsh-bot commented Jun 5, 2015

b9e611c success - wakame-ci/rspec

@axsh-bot
Member
axsh-bot commented Jun 5, 2015

b9e611c success - wakame-ci/rpmbuild

@axsh-bot
Member
axsh-bot commented Jun 5, 2015

b9e611c success - wakame-ci/to-s3

@unakatsuo unakatsuo commented on an outdated diff Jun 5, 2015
dcmgr/lib/dcmgr/rpc/windows_handler.rb
@@ -21,11 +21,14 @@ class WindowsHandler < EndpointBuilder
[@hva_ctx]
)
+ after_in_minutes = Dcmgr::Configurations.hva.windows.delete_password_after
+ expire = Time.now + ( after_in_minutes * 60 )
@unakatsuo
unakatsuo Jun 5, 2015 Member

use UTC.

@axsh-bot
Member
axsh-bot commented Jun 5, 2015

b9e611c success - wakame-ci/dummy.smoke

@axsh-bot
Member
axsh-bot commented Jun 5, 2015

b9e611c failure - wakame-ci/lxc.smoke.allowed-failure

@axsh-bot
Member
axsh-bot commented Jun 5, 2015

b9e611c success - wakame-ci/kvm.smoke.allowed-failure

@axsh-bot
Member
axsh-bot commented Jun 5, 2015

b9e611c success - wakame-ci/kvm.smoke

@axsh-bot
Member
axsh-bot commented Jun 5, 2015

b9e611c success - wakame-ci/vz.smoke

@hansode hansode commented on the diff Jun 5, 2015
dcmgr/lib/dcmgr/rpc/windows_handler.rb
@@ -21,11 +21,18 @@ class WindowsHandler < EndpointBuilder
[@hva_ctx]
)
+ after_in_minutes = Dcmgr::Configurations.hva.windows.delete_password_after
+ if after_in_minutes == 0
+ expire = nil
+ else
+ expire = Time.now.utc + ( after_in_minutes * 60 )
+ end
+
@hansode
hansode Jun 5, 2015 Member

if administrator set minus number or string delete_password_after param in hva.conf, what will happen?

@triggers
triggers Jun 8, 2015 Contributor

Fixed with 6d39a9a

"Added validation for delete_password_after hva.conf parameter"

@unakatsuo unakatsuo and 2 others commented on an outdated diff Jun 8, 2015
dcmgr/lib/dcmgr/configurations/hva.rb
@@ -45,6 +45,13 @@ class Windows < Fuguta::Configuration
param :password_generation_sleeptime, default: 2
param :password_generation_timeout, default: 60 * 15
+
+ # The time in minutes for the Windows password to remain in
+ # the database. This timer starts running the moment the
+ # password is first inserted in the database. When this time
+ # is expired, the password will be deleted. Set to 0 for
+ # unlimited.
+ param :delete_password_after, default: 0
@unakatsuo
unakatsuo Jun 8, 2015 Member

I think it would be good in second time precision.

delete_password_after_sec
@Metallion
Metallion Jun 9, 2015 Member

I agree this is a better name. Sorry, I misread. It's not just a name change being suggested but a precision suggestion. Still, I think @unakatsuo might be right.

@triggers
triggers Jun 9, 2015 Contributor

Except that the parameter specifies minutes. How about delete_password_after_minutes?

@unakatsuo unakatsuo and 1 other commented on an outdated diff Jun 8, 2015
dcmgr/lib/dcmgr/endpoints/12.03/instances.rb
@@ -186,8 +186,15 @@ def set_monitoring_parameters(instance)
get '/:instance_id/password' do
@instance = find_by_uuid(:Instance, params[:instance_id])
- respond_with(R::InstancePassword.new(@instance).generate)
- end
+ theresponse = R::InstancePassword.new(@instance).generate
+
+ if Dcmgr::Configurations.dcmgr.windows.delete_password_on_request
+ logger.info "Deleting encrypted_password for #{:instance_id}"
+ @instance.set({ :encrypted_password => nil }).save_changes
@unakatsuo
unakatsuo Jun 8, 2015 Member

it does not have to remove the password in GET. it is just enough to return nil value in the response hash data.

since you have two different version of set password nil, here misses to set nil to :password_will_be_deleted_at.
please make the method in Models::Instance to clear them.

@triggers
triggers Jun 8, 2015 Contributor

Something like this? c589c0c

(I'm about to test it now...)
@unakatsuo

triggers added some commits Jun 8, 2015
@triggers triggers Fixed Windows password deletion query and maybe made it safer with NU…
…LL test
123dc94
@triggers triggers Added validation for delete_password_after hva.conf parameter 6d39a9a
@triggers triggers Moved deletion of Windows password to instance model
c589c0c
@Metallion Metallion and 1 other commented on an outdated diff Jun 9, 2015
dcmgr/lib/dcmgr/models/instance.rb
@@ -37,6 +37,7 @@ class Instance < AccountResource
subset(:runnings, {:state => STATE_RUNNING})
subset(:running_or_initializing, {:state => [STATE_RUNNING, STATE_INITIALIZING]})
subset(:stops, {:state => STATE_STOPPED})
+ subset(:expired) { password_will_be_deleted_at <= Time.now.utc }
@Metallion
Metallion Jun 9, 2015 Member

I think the word expired isn't a clear description. How about :password_deletion_time_passed? It's long but more descriptive.

@triggers
triggers Jun 9, 2015 Contributor

Agree. Changed with: f15a240

@Metallion Metallion commented on the diff Jun 9, 2015
dcmgr/lib/dcmgr/node_modules/maintenance.rb
@@ -7,12 +7,14 @@ class Maintenance < Isono::NodeModules::Base
@thread_pool = Isono::ThreadPool.new(1, 'Maintenance')
@thread_pool.pass {
myinstance.dup.expire_ip_handles
+ myinstance.dup.automatic_windows_password_deletion
@Metallion
Metallion Jun 9, 2015 Member

What is the reason for dup here?

@triggers
triggers Jun 9, 2015 Contributor

I wondered the same thing. Copying Jari's code example, which apparently has been working OK.

@Metallion Metallion commented on an outdated diff Jun 9, 2015
dcmgr/lib/dcmgr/node_modules/maintenance.rb
@@ -32,6 +34,15 @@ def expire_ip_handles
}
end
+ def automatic_windows_password_deletion
+ # logger.debug "Checking for windows passwords to delete"
@Metallion
Metallion Jun 9, 2015 Member

leftover commented code like this should be deleted.

@Metallion Metallion commented on an outdated diff Jun 9, 2015
dcmgr/lib/dcmgr/node_modules/maintenance.rb
@@ -32,6 +34,15 @@ def expire_ip_handles
}
end
+ def automatic_windows_password_deletion
+ # logger.debug "Checking for windows passwords to delete"
+
+ Dcmgr::Models::Instance.dataset.alives.where('password_will_be_deleted_at IS NOT NULL').expired.each { |handle|
@Metallion
Metallion Jun 9, 2015 Member

Remarks here.

  1. The lines are a little long. We should try to shorten it if possible. I think 80 colons is a good guide line. A little longer is no problem but it shouldn't be too much longer so horizontal scrolling or wrapped lines can be avoided when using split screens in editors.

  2. I think it's better not to write SQL code directly if it can be avoided. Though not a problem in this particular case, it's bad practice that could allow for SQL injection if used wrongly. Another reason to avoid it is to stay consistent with the rest of the code base.

  3. The variable name handle isn't a good name. We're getting an object of Dcmgr::Models::Instance for every iteration so we should call it inst or instance.

I would refactor it like this.

expired_insts = Dcmgr::Models::Instance.alives.password_deletion_time_passed
expired_insts.exclude(password_will_be_deleted_at: nil).each { |inst|
  logger.debug "Automatically deleting encrypted password for instance with uuid: %s" %
    inst.canonical_uuid
  inst.delete_windows_password
}
triggers added some commits Jun 9, 2015
@triggers triggers Changed "expired" to a more descriptive "password_deletion_time_passed" f15a240
@triggers triggers Refactored sequel code to remove SQL and consolidate tests a96a10f
@triggers triggers Refactored maintenance.rb to reduce line length and better var names 8cc0cb5
@triggers triggers Renamed hva parameter to delete_password_after_minutes
e94d6c5
@axsh-bot
Member
axsh-bot commented Jun 9, 2015

e94d6c5 success - wakame-ci/rspec

@axsh-bot
Member
axsh-bot commented Jun 9, 2015

e94d6c5 success - wakame-ci/rpmbuild

@axsh-bot
Member
axsh-bot commented Jun 9, 2015

e94d6c5 success - wakame-ci/to-s3

@axsh-bot
Member
axsh-bot commented Jun 9, 2015

e94d6c5 success - wakame-ci/dummy.smoke

@axsh-bot
Member
axsh-bot commented Jun 9, 2015

e94d6c5 success - wakame-ci/kvm.smoke.allowed-failure

@axsh-bot
Member
axsh-bot commented Jun 9, 2015

e94d6c5 success - wakame-ci/kvm.smoke

@axsh-bot
Member
axsh-bot commented Jun 9, 2015

e94d6c5 failure - wakame-ci/lxc.smoke.allowed-failure

@axsh-bot
Member
axsh-bot commented Jun 9, 2015

e94d6c5 success - wakame-ci/vz.smoke

@Metallion
Member

Though I'm still interested in the reason for dup, this looks good to me merged right now. +1

@Metallion Metallion merged commit 1931e3e into master Jun 10, 2015

7 of 8 checks passed

wakame-ci/lxc.smoke.allowed-failure The build was failure on wakame-ci #23477 (e94d6c5a).
wakame-ci/dummy.smoke The build was success on wakame-ci #23464 (e94d6c5a).
wakame-ci/kvm.smoke The build was success on wakame-ci #23476 (e94d6c5a).
wakame-ci/kvm.smoke.allowed-failure The build was success on wakame-ci #23475 (e94d6c5a).
wakame-ci/rpmbuild The build was success on wakame-ci #23450 (e94d6c5a).
wakame-ci/rspec The build was success on wakame-ci #23445 (e94d6c5a).
wakame-ci/to-s3 The build was success on wakame-ci #23452 (e94d6c5a).
wakame-ci/vz.smoke The build was success on wakame-ci #23478 (e94d6c5a).
@Metallion Metallion deleted the windows-password-auto-deletion branch Jun 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment