Skip to content

[security] Sensitive type for gitlab_rails #487

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
m8t wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[security] Sensitive type for gitlab_rails #487

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fd37f0
[security] Sensitive type for gitlab_rails
Feb 16, 2026
efa1f99
Call unwrap through Puppet manifest
Feb 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion REFERENCE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ Default value: `undef`

##### <a name="-gitlab--gitlab_rails"></a>`gitlab_rails`

Data type: `Optional[Hash]`
Data type: `Optional[Variant[Hash,Sensitive[Hash]]]`

Hash of 'gitlab_pages' config parameters.

Expand Down
2 changes: 1 addition & 1 deletion manifests/init.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@
Optional[Hash] $gitlab_ci = undef,
Optional[Hash] $gitlab_kas = undef,
Optional[Hash] $gitlab_pages = undef,
Optional[Hash] $gitlab_rails = undef,
Optional[Variant[Hash,Sensitive[Hash]]] $gitlab_rails = undef,
Optional[Hash] $gitlab_sshd = undef,
Optional[Hash] $grafana = undef,
Optional[Hash] $high_availability = undef,
Expand Down
13 changes: 10 additions & 3 deletions templates/gitlab.rb.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,21 @@ roles <%= decorate(@roles) %>
git_data_dirs(<%= decorate(@git_data_dirs) %>)

<%- end -%>
<%- if @gitlab_rails -%>
<%-
if @gitlab_rails
if @gitlab_rails.is_a?(Puppet::Pops::Types::PSensitiveType::Sensitive)
Copy link
Copy Markdown
Member

@alexjfisher alexjfisher Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this conditional needed? I thought perhaps for some time, unwrap was a noop if the data wasn't Sensitive?

Copy link
Copy Markdown
Member

@alexjfisher alexjfisher Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

puppet apply -t -e 'notice("foo".unwrap)'
Notice: Scope(Class[main]): foo

Copy link
Copy Markdown
Author

@m8t m8t Feb 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a type String it bailed out with an error, none-existing method

edit fails with type Hash, String is fine in ERB as long as you call it through scope.call_function

Copy link
Copy Markdown
Author

@m8t m8t Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thing is, in a ERB template it fails, in Puppet code it's fine, I think.

Copy link
Copy Markdown
Author

@m8t m8t Feb 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and fails both by calling Puppet function or plain ERB:

gitlab_rails = scope.call_function('unwrap', @gitlab_rails)

  Filepath: /opt/puppetlabs/puppet/lib/ruby/vendor_ruby/puppet/pops/functions/dispatcher.rb
  Line: 40
  Detail: 'unwrap' expects 1 argument, got 16

gitlab_rails = @gitlab_rails.unwrap

  Filepath: /etc/puppetlabs/code/environments/production/modules/gitlab/templates/gitlab.rb.erb
  Line: 68
  Detail: undefined method `unwrap' for #<Hash:0x37327d95>

Copy link
Copy Markdown
Member

@alexjfisher alexjfisher Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and fails both by calling Puppet function or plain ERB:

gitlab_rails = scope.call_function('unwrap', @gitlab_rails)

Close! But the second argument needs to be an array of arguments to the function.
ie scope.call_function('unwrap', [@gitlab_rails]) should work I believe.

Copy link
Copy Markdown
Author

@m8t m8t Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oddly this works fine puppet apply -t -e 'notice({"foo"=>0,"bar"=>1}.unwrap)'

Copy link
Copy Markdown
Author

@m8t m8t Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works fine through Puppet code only to unwrap a type Hash.

diff --git a/manifests/omnibus_config.pp b/manifests/omnibus_config.pp
index bcef084..1720747 100644
--- a/manifests/omnibus_config.pp
+++ b/manifests/omnibus_config.pp
@@ -27,3 +27,3 @@ class gitlab::omnibus_config (
   $gitlab_pages = $gitlab::gitlab_pages
-  $gitlab_rails = $gitlab::gitlab_rails
+  $_gitlab_rails = $gitlab::gitlab_rails
   $gitlab_sshd = $gitlab::gitlab_sshd
@@ -121,2 +121,3 @@ class gitlab::omnibus_config (
     } else {
+      $gitlab_rails = $_gitlab_rails.unwrap
       file { $config_file:

works fine. I can send a new patch, avoids having type check in ERB

Copy link
Copy Markdown
Author

@m8t m8t Feb 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Close! But the second argument needs to be an array of arguments to the function. ie scope.call_function('unwrap', [@gitlab_rails]) should work I believe.

Ah true, thought about it, and forgot, I prefer this variant. I already sent a different commit, where unwrap is called from the manifest, reused in the template. In the end both are fine, and the odd condition around type checking is not needed.

Copy link
Copy Markdown
Member

@alexjfisher alexjfisher Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either works for me. I've approved it as-is, but if you wanted to change it, I'm happy to reapprove.

gitlab_rails = @gitlab_rails.unwrap
else
gitlab_rails = @gitlab_rails
end
-%>

############################
# gitlab.yml configuration #
############################

<%- @gitlab_rails.keys.sort.each do |k| -%>
gitlab_rails['<%= k -%>'] = <%= decorate(@gitlab_rails[k]) %>
<%- gitlab_rails.keys.sort.each do |k| -%>
gitlab_rails['<%= k -%>'] = <%= decorate(gitlab_rails[k]) %>
<%- end end -%>
<%- if @user -%>

Expand Down