Re: [Spacewalk-devel] [PATCH] Has signed Metadata

2011-07-19 Thread Jan Pazdziora 
On Tue, May 24, 2011 at 12:24:53PM +0200, Michael Calmer wrote:
 
 I re-worked the patches now. 
 
 spacewalk-repo-sync uses now a fix directory with a GPG keyring to store 
 trusted keys. 

Michael,

I'm not quite happy about two things:

- The interactiveness of the approach. Neither satellite-sync nor 
spacewalk-repo-sync were interactive in the past and I'm worried that 
making it interactive now could break expectations for people who run
these from quartz or from cron jobs, when the scripts start to do some 
interactive manipulations.

Also, this approach kinda assumes that you are a root on the Spacewalk
server to hit that y to allow the key to be imported. But with
spacewalk-repo-sync and the sync (and scheduled syncs), we finally were
able to separate the root on the Spacewalk server from the content
admin -- you can now define channel and repository *and schedule sync
of the content* from the WebUI, without even knowing the root
password.

I wouldn't like us to lose this.

- I don't quite understand why you use filesystem to store the pubring,
why not put the keys to the database. That would make it much easier
to manage them, for example to ack import of the key from the WebUI.
When the distribution is being created, we could fetch the repomd.xml
and the key right on the spot and let the user approve it right there,
before the first sync.

The biggest problem with your approach is the fact that repositories
and channels are per-organization but the gpgdir in your patch is
global.

So again, it gets us back to the you have to be root to manage
things approach.

We already have a GPG and SSL Keys application which stores the keys
in the rhnCryptoKey table, that even has the org_id column. Can't you
just use this table to store the keys, instead of the gpgdir?

Also, is it correct that there is a boolean in the rhnContentSource,
instead of have a relation table which would list allowed and expected
keys (in rhnCryptoKey) for each rhnContentSource? Essentially have
per-rhnContentSource keyring. With the current boolean approach, I
might have a key for some external repository that I don't trust much,
mixed with key for my primary, most important content.

-- 
Jan Pazdziora
Principal Software Engineer, Satellite Engineering, Red Hat

___
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel

Re: [Spacewalk-devel] [PATCH] Has signed Metadata

2011-05-24 Thread Michael Calmer 
Hi,

Am Montag, 21. März 2011, 15:56:50 schrieb Michael Mraka:
 Michael Calmer wrote:
 % Hi,
 ...
 % 
 % I would suggest, that we go step by step. So let's wait until weak deps
 % and 
 % updateinfo to errata is applied. After this I will submit a new patch
 % for this feature based on the new master.
 
 Sure, no problem.

I re-worked the patches now. 

spacewalk-repo-sync uses now a fix directory with a GPG keyring to store 
trusted keys. 

Please review. Thanks.

-- 
Regards

Michael Calmer

--
Michael Calmer
SUSE LINUX Products GmbH, Maxfeldstr. 5, D-90409 Nuernberg
T: +49 (0) 911 74053 0
F: +49 (0) 911 74053575  - e-mail: michael.cal...@suse.com
--
SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer
HRB 16746 (AG Nürnberg)
From d7c0e9d8ae3684566ae79573d1e8e369e2388323 Mon Sep 17 00:00:00 2001
From: Michael Calmer m...@suse.de
Date: Sat, 21 May 2011 15:56:47 +0200
Subject: [PATCH 1/3] add column metadata_signed to rhnContentSource table

---
 .../spacewalk/common/tables/rhnContentSource.sql   |4 
 .../019-add-metadata_signed.sql|   16 
 2 files changed, 20 insertions(+), 0 deletions(-)
 create mode 100644 schema/spacewalk/upgrade/spacewalk-schema-1.4-to-spacewalk-schema-1.5/019-add-metadata_signed.sql

diff --git a/schema/spacewalk/common/tables/rhnContentSource.sql b/schema/spacewalk/common/tables/rhnContentSource.sql
index dc93a27..2f36f2e 100644
--- a/schema/spacewalk/common/tables/rhnContentSource.sql
+++ b/schema/spacewalk/common/tables/rhnContentSource.sql
@@ -29,6 +29,10 @@ rhnContentSource
 references rhnContentSourceType(id),
 source_url  varchar2(512) NOT NULL,
 label   varchar2(64) NOT NULL,
+metadata_signed CHAR(1)
+DEFAULT ('N') NOT NULL
+CONSTRAINT rhn_cs_ms_ck
+CHECK (metadata_signed in ( 'Y' , 'N' )),
 created date default(sysdate) NOT NULL,
 modifieddate default(sysdate) NOT NULL
 )
diff --git a/schema/spacewalk/upgrade/spacewalk-schema-1.4-to-spacewalk-schema-1.5/019-add-metadata_signed.sql b/schema/spacewalk/upgrade/spacewalk-schema-1.4-to-spacewalk-schema-1.5/019-add-metadata_signed.sql
new file mode 100644
index 000..8839641
--- /dev/null
+++ b/schema/spacewalk/upgrade/spacewalk-schema-1.4-to-spacewalk-schema-1.5/019-add-metadata_signed.sql
@@ -0,0 +1,16 @@
+--
+-- Copyright (c) 2011 SUSE LINUX Products GmbH
+--
+-- This software is licensed to you under the GNU General Public License,
+-- version 2 (GPLv2). There is NO WARRANTY for this software, express or
+-- implied, including the implied warranties of MERCHANTABILITY or FITNESS
+-- FOR A PARTICULAR PURPOSE. You should have received a copy of GPLv2
+-- along with this software; if not, see
+-- http://www.gnu.org/licenses/old-licenses/gpl-2.0.txt.
+--
+--
+alter table rhnContentSource
+add metadata_signed char(1) default('N')
+constraint rhn_cs_ms_nn not null
+constraint rhn_cs_ms_ck
+check (metadata_signed in ('Y','N'));
-- 
1.7.3.4

From 77ea4ace147ac72ebc2834bb8954b53198278da5 Mon Sep 17 00:00:00 2001
From: Michael Calmer m...@suse.de
Date: Sat, 21 May 2011 16:17:29 +0200
Subject: [PATCH 2/3] add has signed metadata checkbox to manage repositories page

---
 .../rhn/domain/channel/ContentSource.hbm.xml   |1 +
 .../redhat/rhn/domain/channel/ContentSource.java   |   17 
 .../channel/manage/repo/RepoDetailsAction.java |   16 +++
 .../frontend/strings/jsp/StringResource_en_US.xml  |3 ++
 .../rhn/manager/channel/repo/BaseRepoCommand.java  |   21 
 .../redhat/rhn/taskomatic/task/RepoSyncTask.java   |1 +
 .../pages/channel/manage/repo/repodetails.jsp  |8 +++
 java/code/webapp/WEB-INF/struts-config.xml |1 +
 8 files changed, 68 insertions(+), 0 deletions(-)

diff --git a/java/code/src/com/redhat/rhn/domain/channel/ContentSource.hbm.xml b/java/code/src/com/redhat/rhn/domain/channel/ContentSource.hbm.xml
index 79ad517..39bf85f 100644
--- a/java/code/src/com/redhat/rhn/domain/channel/ContentSource.hbm.xml
+++ b/java/code/src/com/redhat/rhn/domain/channel/ContentSource.hbm.xml
@@ -17,6 +17,7 @@ PUBLIC -//Hibernate/Hibernate Mapping DTD 3.0//EN
 property name=created type=timestamp column=created  insert=false update=false /
 property name=modified type=timestamp column=modified insert=false update=false /
 property name=label type=string column=label/
+property name=metadataSigned type=yes_no column=metadata_signed/
 
 many-to-one name=org
  column=org_id
diff --git a/java/code/src/com/redhat/rhn/domain/channel/ContentSource.java

Re: [Spacewalk-devel] [PATCH] Has signed Metadata

2011-03-21 Thread Michael Mraka 
Michael Calmer wrote:
% Hello,
% 
% the attached patch set implements checking of signed metadata before 
mirroring 
% with spacewalk-repo-sync.
% This patch adds a checkbox in Manage Repositories. If this checkbox is 
% checked, spacewalk-repo-sync expects, that the repo metadata are signed and 
% try to verify the signature with an installed gpg key. If a matching gpg key 
% is not found and the new commandile parameter --non-interactive is given, 
% spacewalk-repo-sync abort with an error.
% if --non-interactive is not given, spacewalk-repo-sync try to download the 
key 
% file from the server, display the values and ask the user to accept the key.
% If the user agree, the key is installed and the signature will be verified.

Hi Michael,

when applying the first of your patches git am yelds whitespace warnings.
Could you please the patch to get rid of them?

  $ git am /tmp/0001-add-column-metadata_signed-to-rhnContentSource-table.patch 
  Applying: add column metadata_signed to rhnContentSource table
  spacewalk.git/.git/rebase-apply/patch:18: trailing whitespace.
CHECK (metadata_signed in ( 'Y' , 'N' )), 
  spacewalk.git/.git/rebase-apply/patch:27: new blank line at EOF.
  +
  warning: 1 line applied after fixing whitespace errors.


Regards,

--
Michael Mráka
Satellite Engineering, Red Hat

___
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel

Re: [Spacewalk-devel] [PATCH] Has signed Metadata

2011-03-21 Thread Michael Mraka 
Michael Calmer wrote:
% Hello,
% 
% the attached patch set implements checking of signed metadata before 
mirroring 
% with spacewalk-repo-sync.
% This patch adds a checkbox in Manage Repositories. If this checkbox is 
% checked, spacewalk-repo-sync expects, that the repo metadata are signed and 
% try to verify the signature with an installed gpg key. If a matching gpg key 
% is not found and the new commandile parameter --non-interactive is given, 
% spacewalk-repo-sync abort with an error.
% if --non-interactive is not given, spacewalk-repo-sync try to download the 
key 
% file from the server, display the values and ask the user to accept the key.
% If the user agree, the key is installed and the signature will be verified.

Hi Michael,

your second patch failed to apply on today's master HEAD (namely
b9d72a78d8084df93c90cd365ca24b776306eb15). 

I tried to apply it on older base - from Mon 7 Mar 2011 when you sent
the patches - to get rid of changes we made later. Unfortunately
it failed to apply even there.
Could you please rebase your patch to the lastest spacewalk master?


  $ git am /tmp/0002-implement-metadata-signature-checking-for-repositori.patch 
  Applying: implement metadata signature checking for repositories
  spacewalk.git/.git/rebase-apply/patch:68: trailing whitespace.
  
  spacewalk.git/.git/rebase-apply/patch:200: trailing whitespace.
   
  error: patch failed: backend/satellite_tools/repo_plugins/yum_src.py:16
  error: backend/satellite_tools/repo_plugins/yum_src.py: patch does not apply
  error: patch failed: backend/satellite_tools/reposync.py:29
  error: backend/satellite_tools/reposync.py: patch does not apply
  Patch failed at 0001 implement metadata signature checking for repositories
  When you have resolved this problem run git am --resolved.
  If you would prefer to skip this patch, instead run git am --skip.
  To restore the original branch and stop patching run git am --abort.

Thank you.

Regards,

--
Michael Mráka
Satellite Engineering, Red Hat

___
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel

Re: [Spacewalk-devel] [PATCH] Has signed Metadata

2011-03-21 Thread Michael Mraka 
Michael Mraka wrote:
% Michael Calmer wrote:
% % Hello,
% % 
% % the attached patch set implements checking of signed metadata before 
mirroring 
% % with spacewalk-repo-sync.
% % This patch adds a checkbox in Manage Repositories. If this checkbox is 
% % checked, spacewalk-repo-sync expects, that the repo metadata are signed and 
% % try to verify the signature with an installed gpg key. If a matching gpg 
key 
% % is not found and the new commandile parameter --non-interactive is given, 
% % spacewalk-repo-sync abort with an error.
% % if --non-interactive is not given, spacewalk-repo-sync try to download the 
key 
% % file from the server, display the values and ask the user to accept the key.
% % If the user agree, the key is installed and the signature will be verified.
% 
% Hi Michael,
% 
% your second patch failed to apply on today's master HEAD (namely
% b9d72a78d8084df93c90cd365ca24b776306eb15). 
% 
% I tried to apply it on older base - from Mon 7 Mar 2011 when you sent
% the patches - to get rid of changes we made later. Unfortunately
% it failed to apply even there.
% Could you please rebase your patch to the lastest spacewalk master?
% 
% 
%   $ git am 
/tmp/0002-implement-metadata-signature-checking-for-repositori.patch 
%   Applying: implement metadata signature checking for repositories
%   spacewalk.git/.git/rebase-apply/patch:68: trailing whitespace.
%   
%   spacewalk.git/.git/rebase-apply/patch:200: trailing whitespace.
%
%   error: patch failed: backend/satellite_tools/repo_plugins/yum_src.py:16

Michael,

I'm trying apply 0002 patch manually and the error is

  patching file backend/satellite_tools/repo_plugins/yum_src.py
  Hunk #1 FAILED at 16.
  Hunk #2 succeeded at 32 (offset -3 lines).
  Hunk #3 succeeded at 64 (offset -3 lines).
  Hunk #4 FAILED at 116.

In the hunk #1 there is 

% diff --git a/backend/satellite_tools/repo_plugins/yum_src.py 
b/backend/satellite_tools/repo_plugins/yum_src.py
% index 782d49d..3b52d0a 100644
% --- a/backend/satellite_tools/repo_plugins/yum_src.py
% +++ b/backend/satellite_tools/repo_plugins/yum_src.py
% @@ -16,10 +16,18 @@
%  import yum
%  import shutil
%  import sys
% +import os
%  from yum import config
%  from yum.update_md import UpdateMetadata
% -from spacewalk.satellite_tools.reposync import ContentPackage
...

but there aren't lines 'from yum import config' and 'from yum.update_md import 
UpdateMetadata' so you probably missed some older commit which is required by 
your patch.

Similary hunk #4 starts with

  @@ -106,3 +124,142 @@ class ContentSource:
 um = UpdateMetadata()
 um.add(self.repo, all=True)
 return um.notices

which code is missing in spacewalk master.


Regards,

--
Michael Mráka
Satellite Engineering, Red Hat

___
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel

Re: [Spacewalk-devel] [PATCH] Has signed Metadata

2011-03-21 Thread Michael Calmer 
Hi,

Am Montag, 21. März 2011, 15:28:49 schrieb Michael Mraka:
 Michael Mraka wrote:
 % Michael Calmer wrote:
 % % Hello,
 % % 
 % Hi Michael,
 % 
 % your second patch failed to apply on today's master HEAD (namely
 % b9d72a78d8084df93c90cd365ca24b776306eb15). 
 % 
 % I tried to apply it on older base - from Mon 7 Mar 2011 when you sent
 % the patches - to get rid of changes we made later. Unfortunately
 % it failed to apply even there.
 % Could you please rebase your patch to the lastest spacewalk master?

yes, I need to re-do the patch. 
 
 % 
 %   $ git am /tmp/0002-implement-metadata-signature-checking-for-
repositori.patch 
 %   Applying: implement metadata signature checking for repositories
 %   spacewalk.git/.git/rebase-apply/patch:68: trailing whitespace.
 %   
 %   spacewalk.git/.git/rebase-apply/patch:200: trailing whitespace.
 %
 %   error: patch failed: backend/satellite_tools/repo_plugins/yum_src.py:16
 
 Michael,
 
 I'm trying apply 0002 patch manually and the error is
 
   patching file backend/satellite_tools/repo_plugins/yum_src.py
   Hunk #1 FAILED at 16.
   Hunk #2 succeeded at 32 (offset -3 lines).
   Hunk #3 succeeded at 64 (offset -3 lines).
   Hunk #4 FAILED at 116.
 
 In the hunk #1 there is 
 
 % diff --git a/backend/satellite_tools/repo_plugins/yum_src.py 
b/backend/satellite_tools/repo_plugins/yum_src.py
 % index 782d49d..3b52d0a 100644
 % --- a/backend/satellite_tools/repo_plugins/yum_src.py
 % +++ b/backend/satellite_tools/repo_plugins/yum_src.py
 % @@ -16,10 +16,18 @@
 %  import yum
 %  import shutil
 %  import sys
 % +import os
 %  from yum import config

It seems this was there in older spacewalk versions and removed some time ago, 
but it was still in our git and by accident re-added in the updateinfo to 
Errata patch set.

 %  from yum.update_md import UpdateMetadata
 % -from spacewalk.satellite_tools.reposync import ContentPackage
 ...
 
 but there aren't lines 'from yum import config' and 'from yum.update_md
 import UpdateMetadata' so you probably missed some older commit which is
 required by your patch.
 
 Similary hunk #4 starts with
 
   @@ -106,3 +124,142 @@ class ContentSource:
  um = UpdateMetadata()
  um.add(self.repo, all=True)
  return um.notices
 
 which code is missing in spacewalk master.

yes, all this is part of the updateinfo to Errata patch.


I would suggest, that we go step by step. So let's wait until weak deps and 
updateinfo to errata is applied. After this I will submit a new patch for 
this feature based on the new master.

-- 
Regards,

Michael Calmer

--
Michael Calmer
SUSE LINUX Products GmbH, Maxfeldstr. 5, D-90409 Nuernberg
T: +49 (0) 911 74053 0
F: +49 (0) 911 74053575  - e-mail: michael.cal...@suse.com
--
SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg)

___
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel

Re: [Spacewalk-devel] [PATCH] Has signed Metadata

2011-03-21 Thread Michael Mraka 
% % Hello,
% % 
% % the attached patch set implements checking of signed metadata before 
mirroring 
% % with spacewalk-repo-sync.
% % This patch adds a checkbox in Manage Repositories. If this checkbox is 
% % checked, spacewalk-repo-sync expects, that the repo metadata are signed and 
% % try to verify the signature with an installed gpg key. If a matching gpg 
key 
% % is not found and the new commandile parameter --non-interactive is given, 
% % spacewalk-repo-sync abort with an error.
% % if --non-interactive is not given, spacewalk-repo-sync try to download the 
key 
% % file from the server, display the values and ask the user to accept the key.
% % If the user agree, the key is installed and the signature will be verified.

So it check whether rpms loaded into spacewalk, i.e.
_application_content_, are signed with the key. In this case IMO the gpg
key should never be imported into spacewalk servers _OS_ rpm database.
These are two completely separated things; E.g. syncing Fedora 9 into
spacewalk application should not allow F9 packages to be (successfully)
installed  verified on spacewalk server (the OS).
There should be a different gpg database for spacewalk-repo-sync verification.

+def import_key_to_rpmdb(self, raw, keyid, gpgdir):
+  if not os.path.exists(gpgdir):
+os.makedirs(gpgdir)
+  tmpfile = os.path.join(gpgdir, keyid)
+  fp = open(tmpfile, 'w')
+  fp.write(raw)
+  fp.close()
+  cmd = ['/bin/rpm', '--import', tmpfile]
+  p = subprocess.Popen(cmd)
+  sts = os.waitpid(p.pid, 0)[1]
+  os.remove(tmpfile)
+  if sts == 0:
+return True
+  return False

Regards,

--
Michael Mráka
Satellite Engineering, Red Hat

___
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel

Re: [Spacewalk-devel] [PATCH] Has signed Metadata

2011-03-21 Thread Michael Mraka 
Michael Calmer wrote:
% Hi,
...
%  Similary hunk #4 starts with
%  
%@@ -106,3 +124,142 @@ class ContentSource:
%   um = UpdateMetadata()
%   um.add(self.repo, all=True)
%   return um.notices
%  
%  which code is missing in spacewalk master.
% 
% yes, all this is part of the updateinfo to Errata patch.
% 
% 
% I would suggest, that we go step by step. So let's wait until weak deps and 
% updateinfo to errata is applied. After this I will submit a new patch for 
% this feature based on the new master.

Sure, no problem.

Regards,

--
Michael Mráka
Satellite Engineering, Red Hat

___
Spacewalk-devel mailing list
Spacewalk-devel@redhat.com
https://www.redhat.com/mailman/listinfo/spacewalk-devel