Be more strict for openldap_access resource title

The openldap_access resource allows a lot of variations in the title for declaring a resource, making it possible to skip passing parameters such as `what` and `suffix`. This flexibility however can confuse Puppet when it is prefetching resources, leading to catalog compilation failures. Impose a more strict format for resource titles, and validate it with tighter custom types to raise a hopefully meaningful error instead of a Ruby error because something borked bad.
voxpupuli · Dec 10, 2021 · f37118a · f37118a
1 parent 9ecdd03
commit f37118a
Show file tree

Hide file tree

Showing 15 changed files with 124 additions and 190 deletions.
diff --git a/README.md b/README.md
@@ -150,26 +150,7 @@ openldap::server::overlay { 'memberof on dc=example,dc=com':
 [Documentation](http://www.openldap.org/devel/admin/slapdconf2.html) about olcAcces state the following spec:
 > 5.2.5.2. olcAccess: to &lt;what&gt; \[ by &lt;who&gt; \[&lt;accesslevel&gt;\] \[&lt;control&gt;\] \]+
 
-So we supports natively this way of writing in the title:
-```puppet
-openldap::server::access { 'to attrs=userPassword,shadowLastChange by dn="cn=admin,dc=example,dc=com" write by anonymous auth' :
-  suffix   => 'dc=example,dc=com',
-}
-```
-
-Also is supported writing priority in title like olcAccess in ldap
-```puppet
-openldap::server::access { '{0}to attrs=userPassword,shadowLastChange by dn="cn=admin,dc=example,dc=com" write by anonymous auth' :
-  suffix   => 'dc=example,dc=com',
-}
-```
-
-As a single line with suffix:
-```puppet
-openldap::server::access { '{0}to attrs=userPassword,shadowLastChange by dn="cn=admin,dc=example,dc=com" write by anonymous auth on dc=example,dc=com' : }
-```
-
-Defining priority and suffix in the title:
+Define priority and suffix in the title:
 ```puppet
 openldap::server::access { '0 on dc=example,dc=com':
   what     => 'attrs=userPassword,shadowLastChange',
@@ -200,34 +181,14 @@ openldap::server::access { '0 on cn=frontend' :
 }
 ```
 
-
-#### Note #1:
-The chaining arrows `->` are importants if you want to order your entries.
-Openldap put the entry as the last available position.
-So if you got in your ldap:
-```
- olcAccess: {0}to ...
- olcAccess: {1}to ...
- olcAccess: {2}to ...
-```
-
-  Even if you set the parameter `position => '4'`, the next entry will be set as
-
-```
- olcAccess: {3}to ...
-```
-
-#### Note #2:
+#### Note:
 For purging unmanaged entries, rely on the `resources` resource:
 
 ```
 resources { 'openldap_access':
   purge => true,
 }
-```
 
-It is then necessary to identify access rules using the *priority and suffix* syntax for the title:
-```puppet
 openldap::server::access { '0 on dc=example,dc=com':
   what   => ...,
   access => [...],
@@ -238,11 +199,9 @@ openldap::server::access { '1 on dc=example,dc=com':
 }
 ```
 
-entries 2 and 3 will get deleted.
-
 #### Call your acl from a hash:
 The class `openldap::server::access_wrapper` was designed to simplify creating ACL.
-If you have multiple `what` (`to *` in this example), you can order them by adding number to it.
+In order to avoid collisions when multiple identical `what` are present (`to *` in this example), a (meaningless) number must be prepended to each entry.
 
 ```puppet
 $example_acl = {
@@ -252,12 +211,12 @@ $example_acl = {
     'by dn.exact=cn=replicator,dc=example,dc=com read',
     'by * break',
   ],
-  'to attrs=userPassword,shadowLastChange' => [
+  '2 to attrs=userPassword,shadowLastChange' => [
     'by dn="cn=admin,dc=example,dc=com" write',
     'by self write',
     'by anonymous auth',
   ],
-  '2 to *' => [
+  '3 to *' => [
     'by self read',
   ],
 }

diff --git a/lib/puppet/type/openldap_access.rb b/lib/puppet/type/openldap_access.rb
@@ -44,44 +44,7 @@ def insync?(is)
   end
 
   def self.title_patterns
-    what_re = %r{\S+=\S+(?:\s+\S+=\S+)*}
     [
-      [
-        %r{^(\{(\d+)\}to\s+(#{what_re})\s+(by\s+.+)\s+on\s+(.+))$},
-        [
-          [:name],
-          [:position],
-          [:what],
-          [:access],
-          [:suffix],
-        ],
-      ],
-      [
-        %r{^(\{(\d+)\}to\s+(#{what_re})\s+(by\s+.+))$},
-        [
-          [:name],
-          [:position],
-          [:what],
-          [:access],
-        ],
-      ],
-      [
-        %r{^(to\s+(#{what_re})\s+(by\s+.+)\s+on\s+(.+))$},
-        [
-          [:name],
-          [:what],
-          [:access],
-          [:suffix],
-        ],
-      ],
-      [
-        %r{^(to\s+(#{what_re})\s+(by\s+.+))$},
-        [
-          [:name],
-          [:what],
-          [:access],
-        ],
-      ],
       [
         %r{^((\d+)\s+on\s+(.+))$},
         [
@@ -90,12 +53,6 @@ def self.title_patterns
           [:suffix],
         ],
       ],
-      [
-        %r{(.*)},
-        [
-          [:name],
-        ],
-      ],
     ]
   end
 

diff --git a/manifests/server/access.pp b/manifests/server/access.pp
@@ -1,23 +1,27 @@
 # See README.md for details.
 define openldap::server::access (
-  Optional[Enum['present', 'absent']]  $ensure   = undef,
-  Optional[Variant[Integer,String[1]]] $position = undef, # FIXME We should probably choose Integer or String
-  Optional[String[1]]                  $what     = undef,
-  Optional[String[1]]                  $suffix   = undef,
-  Optional[Array[String[1]]]           $access   = undef,
+  String[1]                            $what,
+  Array[Openldap::Access_rule]         $access,
+  Enum['present', 'absent']            $ensure   = 'present',
+  Optional[Variant[Integer,String[1]]] $position = undef, # FIXME deprecated
+  Optional[String[1]]                  $suffix   = undef, # FIXME deprecated
 ) {
   include openldap::server
 
+  if $position or $suffix {
+    warning('openldap::server::access position and suffix are deprecated')
+  }
+
   Class['openldap::server::service']
   -> Openldap::Server::Access[$title]
   -> Class['openldap::server']
 
   openldap_access { $title:
     ensure   => $ensure,
-    position => $position,
+    position => $position, # FIXME deprecated
     target   => $openldap::server::conffile,
     what     => $what,
-    suffix   => $suffix,
+    suffix   => $suffix, # FIXME deprecated
     access   => $access,
   }
 }
diff --git a/manifests/server/access_wrapper.pp b/manifests/server/access_wrapper.pp
@@ -14,14 +14,14 @@
 #
 #   example:
 #     $acl = {
-#       'to *' => [
+#       '1 to *' => [
 #         'by self write',
 #         'by anonymous read',
 #       ],
 #     }
 #
 define openldap::server::access_wrapper (
-  Hash[String[1],Array[String[1]]] $acl,
+  Hash[Pattern[/\A\d+ to\s/],Array[Openldap::Access_rule]] $acl,
   String[1] $suffix = $name,
 ) {
   # Parse ACL

diff --git a/manifests/server/iterate_access.pp b/manifests/server/iterate_access.pp
@@ -1,6 +1,6 @@
 #  This is a 'private' class used by openldap::server::access_wrapper
 define openldap::server::iterate_access (
-  Hash $hash,
+  Openldap::Access_hash $hash,
 ) {
   # Call individual openldap::server::access
   $position = $hash[$name]['position']

diff --git a/spec/acceptance/openldap__server__access_spec.rb b/spec/acceptance/openldap__server__access_spec.rb
@@ -26,20 +26,17 @@ class { 'openldap::server': }
       pp = <<-EOS
         class { 'openldap::server': }
         openldap::server::database { 'dc=example,dc=com' : }
-        openldap::server::access { 'admin':
+        openldap::server::access { '0 on dc=example,dc=com':
           what    => 'attrs=userPassword,distinguishedName',
           access  => ['by dn="cn=admin,dc=example,dc=com" write'],
-          suffix  => 'dc=example,dc=com',
           require => Openldap::Server::Database['dc=example,dc=com'],
         }
-        openldap::server::access { 'root':
+        openldap::server::access { '1 on dc=example,dc=com':
           what    => '*',
           access  => [
             'by dn.exact=gidNumber=0+uidNumber=0,cn=peercred,cn=external,cn=auth manage',
             'by * break'
           ],
-          suffix  => 'dc=example,dc=com',
-          position => 0,
           require => Openldap::Server::Database['dc=example,dc=com'],
         }
       EOS

diff --git a/spec/defines/openldap_server_access_spec.rb b/spec/defines/openldap_server_access_spec.rb
@@ -1,67 +1,34 @@
 require 'spec_helper'
 
 describe 'openldap::server::access' do
-  let(:title) { 'foo' }
+  let(:title) { '0 on dc=example,dc=com' }
 
   on_supported_os.each do |os, facts|
     context "on #{os}" do
       let(:facts) do
         facts
       end
 
-      context 'with composite namevar' do
-        let(:title) do
-          'to attrs=userPassword,shadowLastChange by dn="cn=admin,dc=example,dc=com" on dc=example,dc=com'
-        end
-
-        it { is_expected.to compile.with_all_deps }
-        it {
-          is_expected.to contain_openldap_access('to attrs=userPassword,shadowLastChange by dn="cn=admin,dc=example,dc=com" on dc=example,dc=com')
-        }
-      end
-
-      context 'with composite namevar, what includes dn and filter' do
-        let(:title) do
-          'to dn.one="ou=users,dc=example,dc=com" filter=(objectClass=person) by dn="cn=admin,dc=example,dc=com" on dc=example,dc=com'
-        end
-
-        it { is_expected.to compile.with_all_deps }
-        it {
-          is_expected.to contain_openldap_access('to dn.one="ou=users,dc=example,dc=com" filter=(objectClass=person) by dn="cn=admin,dc=example,dc=com" on dc=example,dc=com')
-        }
-      end
-
-      context 'with composite namevar, what includes dn and attrs' do
-        let(:title) do
-          'to dn.one="ou=users,dc=example,dc=com" attrs=userPassword,shadowLastChange by dn="cn=admin,dc=example,dc=com" on dc=example,dc=com'
+      context 'with valid parameters' do
+        let(:params) do
+          {
+            what: 'to *',
+            access: [
+              'by * read',
+            ],
+          }
         end
 
         it { is_expected.to compile.with_all_deps }
-        it {
-          is_expected.to contain_openldap_access('to dn.one="ou=users,dc=example,dc=com" attrs=userPassword,shadowLastChange by dn="cn=admin,dc=example,dc=com" on dc=example,dc=com')
-        }
-      end
-
-      context 'with composite namevar, what includes dn, filter and attrs' do
-        let(:title) do
-          'to dn.one="ou=users,dc=example,dc=com" filter=(objectClass=person) attrs=userPassword,shadowLastChange by dn="cn=admin,dc=example,dc=com" on dc=example,dc=com'
-        end
+        it { is_expected.to contain_openldap_access('0 on dc=example,dc=com') }
 
-        it { is_expected.to compile.with_all_deps }
-        it {
-          is_expected.to contain_openldap_access('to dn.one="ou=users,dc=example,dc=com" filter=(objectClass=person) attrs=userPassword,shadowLastChange by dn="cn=admin,dc=example,dc=com" on dc=example,dc=com')
-        }
-      end
+        context 'with malformed namevar' do
+          let(:title) do
+            'to attrs=userPassword,shadowLastChange by dn="cn=admin,dc=example,dc=com" on dc=example,dc=com'
+          end
 
-      context 'with composite namevar with position' do
-        let(:title) do
-          '{0}to attrs=userPassword,shadowLastChange by dn="cn=admin,dc=example,dc=com" on dc=example,dc=com'
+          it { is_expected.not_to compile.with_all_deps }
         end
-
-        it { is_expected.to compile.with_all_deps }
-        it {
-          is_expected.to contain_openldap_access('{0}to attrs=userPassword,shadowLastChange by dn="cn=admin,dc=example,dc=com" on dc=example,dc=com')
-        }
       end
 
       context 'with access as an array' do
@@ -79,7 +46,7 @@
 
         it { is_expected.to compile.with_all_deps }
         it {
-          is_expected.to contain_openldap_access('foo').
+          is_expected.to contain_openldap_access('0 on dc=example,dc=com').
             with_position('0').
             with_what('to attrs=userPassword,shadowLastChange').
             with_suffix('dc=example,dc=com').

diff --git a/spec/defines/openldap_server_access_wrapper_spec.rb b/spec/defines/openldap_server_access_wrapper_spec.rb
@@ -12,13 +12,13 @@
       let(:params) do
         {
           acl: {
-            '1 to *' => [
+            '0 to *' => [
               'by dn.exact=gidNumber=0+uidNumber=0,cn=peercred,cn=external,cn=auth manage',
               'by dn.exact=cn=admin,dc=example,dc=com write',
               'by dn.exact=cn=replicator,dc=example,dc=com read',
               'by * break',
             ],
-            'to attrs=userPassword,shadowLastChange' => [
+            '1 to attrs=userPassword,shadowLastChange' => [
               'by dn="cn=admin,dc=example,dc=com" write',
               'by self write',
               'by anonymous auth',

diff --git a/spec/type_aliases/access_hash_spec.rb b/spec/type_aliases/access_hash_spec.rb
@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Openldap::Access_hash' do
+  context 'valid types' do
+    [
+      {
+        '0 on dc=example,dc=com' => {
+          'what'   => 'attrs=userPassword,shadowLastChange',
+          'access' => [
+            'by dn="cn=admin,dc=example,dc=com" write',
+            'by anonymous auth',
+            'by self write',
+            'by * none',
+          ],
+        },
+      },
+      {
+        '0 on dc=example,dc=com' => {
+          'position' => 3,
+          'suffix'   => 'dc=example,dc=com',
+          'what'     => 'attrs=userPassword,shadowLastChange',
+          'access'   => [
+            'by dn="cn=admin,dc=example,dc=com" write',
+            'by anonymous auth',
+            'by self write',
+            'by * none',
+          ],
+        },
+      },
+    ].each do |value|
+      describe value.inspect do
+        it { is_expected.to allow_value(value) }
+      end
+    end
+  end
+end
diff --git a/spec/type_aliases/access_rule_spec.rb b/spec/type_aliases/access_rule_spec.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Openldap::Access_rule' do
+  context 'valid value' do
+    [
+      'by dn="cn=admin,dc=example,dc=com write',
+      'by anonymous auth',
+    ].each do |value|
+      describe value.inspect do
+        it { is_expected.to allow_value(value) }
+      end
+    end
+  end
+end