Fixes #13870 - encrypts specific settings values in db #3230

amirfefer · 2016-02-24T14:24:14Z

In the following PR the use case came up.

In order to encrypt a setting value, set its encrypted flag to true:

self.set('root_pass', N_("Default encrypted root password on provisioned hosts"), nil, N_('Root password')), nil, {:encrypted => true})

amirfefer · 2016-02-24T14:25:02Z

app/models/concerns/encrypt_field.rb

@@ -0,0 +1,75 @@
+module EncryptField


Encryptable module encrypts the whole attribute (column), while in some cases there is a need to encrypt a specific field (instance variable). such use cases can be found in settings, and in parameters.
hence encryptable module has splitted into two modules, the first one is encrypt_field, which can be included to models in case of specific fields encryption, and the encryptable which keeps the logic for attribute encryption.

Nitpick, can you call this EncryptString or similar? "field" has a similar meaning to "attribute" to me, so I was confused for a while with the terminology.

xprazak2 · 2016-02-29T16:00:35Z

Not sure where it comes from, but I get additional characters attached to my desired setting value, when I try to change it through setter:

amirfefer · 2016-03-15T22:28:20Z

@domcleal An encrypted flag was added to Setting table, instead of using type attribute.

amirfefer · 2016-03-16T10:28:48Z

[test]

domcleal · 2016-03-16T14:19:58Z

The failures are on every combination as DB migration from clean is failing, please take a look at the errors.

domcleal · 2016-03-18T10:40:55Z

Unit test failures remain, please take a look.

Please remember to comment on PRs when you change them, see https://groups.google.com/d/msg/foreman-dev/poyjF8Ig5UE/jaFyZVGOsEUJ.

amirfefer · 2016-03-24T09:40:07Z

@domcleal Tests are failing because unrelated issue in host#edit.

domcleal · 2016-03-29T12:51:29Z

app/models/setting.rb

@@ -100,11 +101,13 @@ def self.method_missing(method, *args)

  def value=(v)
    v = v.to_yaml unless v.nil?
+    v = encrypt_field(v) if defined? encrypted and encrypted


Prefer has_attribute?(:encrypted) perhaps? A comment to say why you're performing this check would be useful - I'm assuming it's to enable DB migrations on older versions?

Prefer && in an expression instead of and.

domcleal · 2016-09-30T08:34:04Z

app/models/concerns/audit_extensions.rb

+      if change.is_a? Array
+        change.map! {|c| c.to_s.start_with?(EncryptValue::ENCRYPTION_PREFIX) ? _("[encrypted]") : c}
+      else
+        audited_changes[name] = _("[encrypted]") if change.to_s.start_with?(EncryptValue::ENCRYPTION_PREFIX)


Using _() in this context will store the translated message in the database, so it'll be stored in the language of whoever made the change instead of whoever is viewing it.

I think perhaps you want to use _N('[encrypted]') here to only extract it, store the English and then call _(db_value) in the helper for display purposes (when db_value == '[encrypted]').

domcleal · 2016-09-30T08:34:46Z

app/models/concerns/audit_extensions.rb

+    self.audited_changes.each do |name,change|
+      next if change.nil? || change.to_s.empty?
+      if change.is_a? Array
+        change.map! {|c| c.to_s.start_with?(EncryptValue::ENCRYPTION_PREFIX) ? _("[encrypted]") : c}


ditto, storing a translated string instead of English

domcleal · 2016-09-30T08:36:12Z

app/helpers/audits_helper.rb

@@ -13,7 +13,7 @@ def id_to_label(name, change)
      when /.*_id$/
        name.classify.gsub('Id','').constantize.find(change).to_label
      else
-        change.to_s
+        change.to_s.include?(EncryptValue::ENCRYPTION_PREFIX) ? _("[encrypted]") : change.to_s


Thanks, that's better, but there aren't any tests for the functionality - please add to test/models/concerns/audit_extensions_test.rb.

domcleal

i18n issue with the stored string, and missing unit test for the audit model changes.

amirfefer · 2016-10-05T12:37:41Z

@domcleal I fixed i18n issue , and added a unit test into Audit model extension.

domcleal · 2016-10-05T14:48:27Z

app/models/concerns/audit_extensions.rb

+      if change.is_a? Array
+        change.map! {|c| c.to_s.start_with?(EncryptValue::ENCRYPTION_PREFIX) ? N_("[encrypted]") : c}
+      else
+        audited_changes[name] = N_("[encrypted]") if change.to_s.start_with?(EncryptValue::ENCRYPTION_PREFIX)


Where does [encrypted] get translated in the UI? My last comment here suggested doing it in a UI helper, but I can't see that in this PR. (See #3230 (comment))

I just forgot to commit the changes in audits_helper.

domcleal · 2016-10-05T14:48:40Z

test/models/concerns/audit_extensions_test.rb

+      assert setting.save
+    end
+    a = Audit.where(auditable_type: 'Setting')
+    assert_equal N_("[encrypted]"), a.last.audited_changes["value"][1]


Remove N_() from here, no need to extract a string in a test.

domcleal · 2016-10-06T07:07:32Z

Thanks, one last thing: the encrypted column itself is being audited so you see it change when this patch is applied, but since it's an internal field it should be excluded. Add it to the audited :exclude list in app/models/settings.rb please.

amirfefer · 2016-10-06T23:52:54Z

@domcleal I inserted encrypted field to the audited :except list in setting.

domcleal · 2016-10-07T11:19:26Z

Merged, thanks @amirfefer.

theforeman-bot added Not yet reviewed Needs testing labels Feb 24, 2016

amirfefer reviewed Feb 24, 2016
View reviewed changes

domcleal added Waiting on contributor and removed Not yet reviewed labels Mar 1, 2016

amirfefer force-pushed the 13870 branch from 25e5868 to 41de7e7 Compare March 15, 2016 22:21

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Mar 15, 2016

domcleal added Waiting on contributor and removed Needs re-review labels Mar 16, 2016

amirfefer force-pushed the 13870 branch from 41de7e7 to c47163b Compare March 17, 2016 08:37

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Mar 17, 2016

amirfefer force-pushed the 13870 branch from c47163b to ef83285 Compare March 17, 2016 11:14

domcleal added Waiting on contributor and removed Needs re-review labels Mar 18, 2016

amirfefer force-pushed the 13870 branch from ef83285 to 96706d0 Compare March 24, 2016 07:48

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Mar 24, 2016

domcleal reviewed Mar 29, 2016
View reviewed changes

theforeman-bot added the Needs testing label Sep 29, 2016

amirfefer force-pushed the 13870 branch from d63a98d to 09dacfa Compare September 29, 2016 21:31

domcleal reviewed Sep 30, 2016

View reviewed changes

domcleal suggested changes Sep 30, 2016

View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels Sep 30, 2016

amirfefer force-pushed the 13870 branch from 09dacfa to d2b5ce5 Compare October 5, 2016 12:12

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 5, 2016

domcleal suggested changes Oct 5, 2016

View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels Oct 5, 2016

amirfefer force-pushed the 13870 branch from d2b5ce5 to b04874b Compare October 5, 2016 17:37

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 5, 2016

domcleal added Waiting on contributor and removed Needs re-review labels Oct 6, 2016

Fixes #13870 - encrypt specific settings values in db

e8528a9

amirfefer force-pushed the 13870 branch from b04874b to e8528a9 Compare October 6, 2016 23:37

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Oct 6, 2016

domcleal approved these changes Oct 7, 2016

View reviewed changes

domcleal merged commit b5eefd9 into theforeman:develop Oct 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #13870 - encrypts specific settings values in db #3230

Fixes #13870 - encrypts specific settings values in db #3230

amirfefer commented Feb 24, 2016 •

edited

Loading

amirfefer Feb 24, 2016

domcleal Mar 29, 2016

xprazak2 commented Feb 29, 2016

amirfefer commented Mar 15, 2016

amirfefer commented Mar 16, 2016

domcleal commented Mar 16, 2016

domcleal commented Mar 18, 2016

amirfefer commented Mar 24, 2016

domcleal Mar 29, 2016

domcleal Mar 29, 2016

domcleal Sep 30, 2016 •

edited

Loading

domcleal Sep 30, 2016

domcleal Sep 30, 2016

domcleal left a comment

amirfefer commented Oct 5, 2016

domcleal Oct 5, 2016

amirfefer Oct 5, 2016

domcleal Oct 5, 2016

domcleal commented Oct 6, 2016

amirfefer commented Oct 6, 2016

domcleal commented Oct 7, 2016

Fixes #13870 - encrypts specific settings values in db #3230

Fixes #13870 - encrypts specific settings values in db #3230

Conversation

amirfefer commented Feb 24, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xprazak2 commented Feb 29, 2016

amirfefer commented Mar 15, 2016

amirfefer commented Mar 16, 2016

domcleal commented Mar 16, 2016

domcleal commented Mar 18, 2016

amirfefer commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcleal Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcleal left a comment

Choose a reason for hiding this comment

amirfefer commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domcleal commented Oct 6, 2016

amirfefer commented Oct 6, 2016

domcleal commented Oct 7, 2016

amirfefer commented Feb 24, 2016 •

edited

Loading

domcleal Sep 30, 2016 •

edited

Loading