WIP

Repo/Service file: Full support for using repo variables in URLs (bsc#1217192) With a libzypp providing 'libzypp(repovarexpand) >= 1.2' the use of repo variables in URLs is no longer restricted. The use of variables is no longer restricted to the host and path component of an URl, nor must the not-expanded string form a valid URL.
openSUSE · Nov 29, 2023 · dc1a9c0 · dc1a9c0
1 parent fd32263
commit dc1a9c0
Show file tree

Hide file tree

Showing 8 changed files with 140 additions and 15 deletions.
diff --git a/doc/autoinclude/FeatureTest.doc b/doc/autoinclude/FeatureTest.doc
@@ -79,6 +79,8 @@ Packages requiring a feature may use the corresponding \c Requires: in their .sp
     <DD>Also support user defined repo variables in /etc/zypp/vars.d.</DD>
     <DT>version 1.1</DT>
     <DD>Support repo variables in an URIs \c host and \c port component.</DD>
+    <DT>version 1.2</DT>
+    <DD>Support repo variables forming valid URIs after expansion - but not before.</DD>
   </DL></DD>
 </DL>
 

diff --git a/doc/autoinclude/RepoVariables.doc b/doc/autoinclude/RepoVariables.doc
@@ -15,7 +15,7 @@ The variable expander also supports shell like definition of default and alterna
 \see \ref zypp::repo::RepoVarExpand Variable expander
 
 
-\subsection zypp-repoars-builtin Builtin repository variables
+\subsection zypp-repovars-builtin Builtin repository variables
 
 \li \c $arch -
 	The system's CPU architecture.
@@ -31,10 +31,11 @@ The variable expander also supports shell like definition of default and alterna
 	\c $releasever_major will be set to the leading portion up to (but not including) the 1st dot; \c $releasever_minor to the trailing portion after the 1st dot. If there's no dot in \c $releasever, \c $releasever_major is the same as \c $releasever and \c $releasever_minor is empty.
 
 
-\subsection zypp-repoars-userdefined User defined repository variables [requires 'libzypp(repovarexpand) >= 1']
+\subsection zypp-repovars-userdefined User defined repository variables [requires 'libzypp(repovarexpand) >= 1']
 
 A custom repository variable is defined by creating a file in \c /etc/zypp/vars.d. The variable name equals the file name. The files first line (up to but not including the newline character) defines the variables value. Valid variable(file) names consist of alphanumeric chars and '_' only.
 
 Variable substitution within an URIs authority [requires 'libzypp(repovarexpand) >= 1.1'] is limited to \c host and \c port. Bash style definition of default and alternate values is not supported. No variables can be used in an URIs \c scheme, \c user and \c password.
 
+The use of arbitrary strings which form a valid url after variable substitution - but not before - requires 'libzypp(repovarexpand) >= 1.2'. The \c zypp-rawurl: schema is used to wrap the unexpanded strings into a valid \ref Url until they get expanded (\see \ref zypp::RawUrl).
 */
diff --git a/libzypp.spec.cmake b/libzypp.spec.cmake
@@ -62,7 +62,7 @@ Provides:       libzypp(plugin:services) = 1
 Provides:       libzypp(plugin:system) = 1
 Provides:       libzypp(plugin:urlresolver) = 0
 Provides:       libzypp(plugin:repoverification) = 0
-Provides:       libzypp(repovarexpand) = 1.1
+Provides:       libzypp(repovarexpand) = 1.2
 
 %if 0%{?suse_version}
 Recommends:     logrotate

diff --git a/tests/parser/RepoFileReader_test.cc b/tests/parser/RepoFileReader_test.cc
@@ -9,6 +9,23 @@ using std::stringstream;
 using std::string;
 using namespace zypp;
 
+// This is kind of ugly for testing: Whenever a zypp lock is
+// acquired the repo variables are cleared and re-read from disk,
+// which is ok (by ZYppFactory).
+// The VarReplacer itself however acquires a lock to resolve the
+// Target variables (like $releasever), which is ok as well.
+// For testing however - if we want to manipulate the variable
+// definitions - we must hold a lock ourselves. Otherwise the
+// VarReplacer will acquired one and so discard our custom
+// settings.
+#include <zypp/ZYppFactory.h>
+auto guard { getZYpp() };
+namespace zyppintern {
+  std::map<std::string,std::string> repoVariablesGet();
+  void repoVariablesSwap( std::map<std::string,std::string> & val_r );
+}
+// ---
+
 static std::string suse_repo = "[factory-oss]\n"
 "name=factory-oss $releasever - $basearch\n"
 "failovermethod=priority\n"
@@ -56,3 +73,78 @@ BOOST_AUTO_TEST_CASE(read_repo_file)
     BOOST_CHECK_EQUAL( Url("http://serv.er/loc1"), repo.mirrorListUrl() );
   }
 }
+
+BOOST_AUTO_TEST_CASE(rawurl2repoinfo)
+{
+  // Set up repo variables...
+  std::map<std::string,std::string> vardef { ::zyppintern::repoVariablesGet() };
+  vardef["releasever"]       = "myversion";
+  vardef["basearch"]         = "myarch";
+  vardef["OPENSUSE_DISTURL"] = "https://cdn.opensuse.org/repositories/";
+  ::zyppintern::repoVariablesSwap( vardef );
+
+  {
+    std::stringstream input(
+      "[leap-repo]\n"
+      "name=leap-repo $releasever - $basearch\n"
+      "baseurl=    ${OPENSUSE_DISTURL}leap/repo\n"
+      "gpgkey=     ${OPENSUSE_DISTURL}leap/repo\n"
+      "mirrorlist= ${OPENSUSE_DISTURL}leap/repo\n"
+      "metalink=   ${OPENSUSE_DISTURL}leap/repo\n"
+    );
+    RepoCollector collector;
+    parser::RepoFileReader parser( input, bind( &RepoCollector::collect, &collector, _1 ) );
+    BOOST_CHECK_EQUAL(1, collector.repos.size());
+
+    const RepoInfo & repo( collector.repos.front() );
+    BOOST_CHECK_EQUAL( repo.name(), "leap-repo myversion - myarch"  );
+
+    BOOST_CHECK_EQUAL( repo.url().asCompleteString(),           "https://cdn.opensuse.org/repositories/leap/repo" );
+    BOOST_CHECK_EQUAL( repo.gpgKeyUrl().asCompleteString(),     "https://cdn.opensuse.org/repositories/leap/repo" );
+    BOOST_CHECK_EQUAL( repo.mirrorListUrl().asCompleteString(), "https://cdn.opensuse.org/repositories/leap/repo" );
+
+    BOOST_CHECK_EQUAL( repo.rawUrl().asCompleteString(),           "zypp-rawurl:#${OPENSUSE_DISTURL}leap/repo" );
+    BOOST_CHECK_EQUAL( repo.rawGpgKeyUrl().asCompleteString(),     "zypp-rawurl:#${OPENSUSE_DISTURL}leap/repo" );
+    BOOST_CHECK_EQUAL( repo.rawMirrorListUrl().asCompleteString(), "zypp-rawurl:#${OPENSUSE_DISTURL}leap/repo" );
+  }
+
+  // RepoFileReader unfortunately encodes "proxy=" into the URLs
+  // query part (by now just for the baseurl). For RawUrls the
+  // VarReplacer needs to take this into account.
+  {
+    std::stringstream input(
+      "[leap-repo]\n"
+      "name=leap-repo $releasever - $basearch\n"
+      "proxy=myproxy.host:1234\n"
+      "baseurl=${OPENSUSE_DISTURL}leap/repo\n"
+      "baseurl=https://cdn.opensuse.org/repositories/leap/repo\n"
+      "baseurl=https://cdn.opensuse.org/repositories/leap/repo?proxy=otherproxy.host\n"
+      "gpgkey=${OPENSUSE_DISTURL}leap/repo\n"
+      "mirrorlist=${OPENSUSE_DISTURL}leap/repo\n"
+      "metalink=${OPENSUSE_DISTURL}leap/repo\n"
+    );
+    RepoCollector collector;
+    parser::RepoFileReader parser( input, bind( &RepoCollector::collect, &collector, _1 ) );
+    BOOST_CHECK_EQUAL(1, collector.repos.size());
+
+    const RepoInfo & repo( collector.repos.front() );
+    BOOST_CHECK_EQUAL( repo.name(), "leap-repo myversion - myarch"  );
+
+    BOOST_CHECK_EQUAL( repo.baseUrlsSize(), 3 );
+    const auto & baseurls = repo.baseUrls();
+    auto iter = baseurls.begin();
+    BOOST_CHECK_EQUAL( (iter++)->asCompleteString(), "https://cdn.opensuse.org/repositories/leap/repo?proxy=myproxy.host&proxyport=1234" );
+    BOOST_CHECK_EQUAL( (iter++)->asCompleteString(), "https://cdn.opensuse.org/repositories/leap/repo?proxy=myproxy.host&proxyport=1234" );
+    BOOST_CHECK_EQUAL( (iter++)->asCompleteString(), "https://cdn.opensuse.org/repositories/leap/repo?proxy=otherproxy.host" );
+    BOOST_CHECK_EQUAL( repo.gpgKeyUrl().asCompleteString(),     "https://cdn.opensuse.org/repositories/leap/repo" );
+    BOOST_CHECK_EQUAL( repo.mirrorListUrl().asCompleteString(), "https://cdn.opensuse.org/repositories/leap/repo" );
+
+    const auto & rawbaseurls = repo.rawBaseUrls();
+    iter = rawbaseurls.begin();
+    BOOST_CHECK_EQUAL( (iter++)->asCompleteString(), "zypp-rawurl:?proxy=myproxy.host&proxyport=1234#${OPENSUSE_DISTURL}leap/repo" );
+    BOOST_CHECK_EQUAL( (iter++)->asCompleteString(), "https://cdn.opensuse.org/repositories/leap/repo?proxy=myproxy.host&proxyport=1234" );
+    BOOST_CHECK_EQUAL( (iter++)->asCompleteString(), "https://cdn.opensuse.org/repositories/leap/repo?proxy=otherproxy.host" );
+    BOOST_CHECK_EQUAL( repo.rawGpgKeyUrl().asCompleteString(),     "zypp-rawurl:#${OPENSUSE_DISTURL}leap/repo" );
+    BOOST_CHECK_EQUAL( repo.rawMirrorListUrl().asCompleteString(), "zypp-rawurl:#${OPENSUSE_DISTURL}leap/repo" );
+  }
+}
diff --git a/zypp/RepoInfo.cc b/zypp/RepoInfo.cc
@@ -884,10 +884,12 @@ namespace zypp
 
   std::ostream & RepoInfo::dumpOn( std::ostream & str ) const
   {
+    // Don't dump raw Urls while the fragment is hidden in the default view
+    // (because it may otherwise expose embedded passwords).
     RepoInfoBase::dumpOn(str);
     if ( _pimpl->baseurl2dump() )
     {
-      for ( const auto & url : _pimpl->baseUrls().raw() )
+      for ( const auto & url : _pimpl->baseUrls().transformed() )  // not .raw()!
       {
         str << "- url         : " << url << std::endl;
       }
@@ -899,7 +901,7 @@ namespace zypp
         str << tag_r << value_r << std::endl;
     });
 
-    strif( (_pimpl->_mirrorListForceMetalink ? "- metalink    : " : "- mirrorlist  : "), rawMirrorListUrl().asString() );
+    strif( (_pimpl->_mirrorListForceMetalink ? "- metalink    : " : "- mirrorlist  : "), mirrorListUrl().asString() ); // not rawMirrorListUrl()
     strif( "- path        : ", path().asString() );
     str << "- type        : " << type() << std::endl;
     str << "- priority    : " << priority() << std::endl;
@@ -913,7 +915,7 @@ namespace zypp
                               << std::endl;
 #undef OUTS
 
-    for ( const auto & url : _pimpl->gpgKeyUrls().raw() )
+    for ( const auto & url : _pimpl->gpgKeyUrls().transformed() )  // not .raw()!
     {
       str << "- gpgkey      : " << url << std::endl;
     }
@@ -932,6 +934,8 @@ namespace zypp
 
   std::ostream & RepoInfo::dumpAsIniOn( std::ostream & str ) const
   {
+    // NOTE: hotfix1050625::asString provides the Url asRawSring,
+    // which is what we want when writing a .repo file.
     RepoInfoBase::dumpAsIniOn(str);
 
     if ( _pimpl->baseurl2dump() )
@@ -970,7 +974,7 @@ namespace zypp
       std::string indent( "gpgkey=");
       for ( const auto & url : _pimpl->gpgKeyUrls().raw() )
       {
-        str << indent << url << endl;
+        str << indent << hotfix1050625::asString( url ) << endl;
         if ( indent[0] != ' ' )
           indent = "       ";
       }

diff --git a/zypp/RepoInfo.h b/zypp/RepoInfo.h
@@ -67,6 +67,15 @@ namespace zypp
    *
    * \note Name, baseUrls and mirrorUrl are subject to repo variable replacement
    * (\see \ref RepoVariablesStringReplacer).
+   *
+   * \note Methods returning RawUrls are deprecated because printing them may
+   * accidentally expose credentials (password,proxypassword) embedded in the
+   * raw url-string. An url-string with embedded RepoVariables, forms a valid
+   * \ref Url after variables have been expanded. In the expanded Url passwords
+   * are protected. The unexpanded url-string however does not even need to form
+   * a valid Url, so sensitive data in the string can not be hidden. This is why
+   * the zypp-rawurl: schema, which is used to store the unexpanded url-strings,
+   * per default prints just it's schema, but not it's content.
    */
   class RepoInfo : public repo::RepoInfoBase
   {
@@ -132,8 +141,9 @@ namespace zypp
       { return( baseUrlsEmpty() ? Url() : *baseUrlsBegin()); }
       /**
        * Pars pro toto: The first repository raw url (no variables replaced)
+       * \deprecated \see \ref RepoInfo
        */
-      Url rawUrl() const;
+      Url rawUrl() const ZYPP_DEPRECATED;
 
       /**
        * The complete set of repository urls
@@ -144,8 +154,9 @@ namespace zypp
       url_set baseUrls() const;
       /**
        * The complete set of raw repository urls (no variables replaced)
+       * \deprecated \see \ref RepoInfo
        */
-      url_set rawBaseUrls() const;
+      url_set rawBaseUrls() const ZYPP_DEPRECATED;
 
       /**
        * Add a base url. \see baseUrls
@@ -197,8 +208,9 @@ namespace zypp
       Url mirrorListUrl() const;
       /**
        * The raw mirrorListUrl (no variables replaced).
+       * \deprecated \see \ref RepoInfo
        */
-      Url rawMirrorListUrl() const;
+      Url rawMirrorListUrl() const ZYPP_DEPRECATED;
       /**
        * Set mirror list url. \see mirrorListUrl
        * \param url The base url for the list
@@ -391,15 +403,19 @@ namespace zypp
 
       /** The list of gpgkey URLs defined for this repo */
       url_set gpgKeyUrls() const;
-      /** The list of raw gpgkey URLs defined for this repo (no variables replaced) */
-      url_set rawGpgKeyUrls() const;
+      /** The list of raw gpgkey URLs defined for this repo (no variables replaced)
+       * \deprecated \see \ref RepoInfo
+       */
+      url_set rawGpgKeyUrls() const ZYPP_DEPRECATED;
       /** Set a list of gpgkey URLs defined for this repo */
       void setGpgKeyUrls( url_set urls );
 
       /** (leagcy API) The 1st gpgkey URL defined for this repo */
       Url gpgKeyUrl() const;
-      /** (leagcy API) The 1st raw gpgkey URL defined for this repo (no variables replaced) */
-      Url rawGpgKeyUrl() const;
+      /** (leagcy API) The 1st raw gpgkey URL defined for this repo (no variables replaced)
+       * \deprecated \see \ref RepoInfo
+       */
+      Url rawGpgKeyUrl() const ZYPP_DEPRECATED;
       /** (leagcy API) Set the gpgkey URL defined for this repo */
       void setGpgKeyUrl( const Url &gpgkey );
 

diff --git a/zypp/parser/RepoFileReader.cc b/zypp/parser/RepoFileReader.cc
@@ -115,7 +115,7 @@ namespace zypp
           // #285: Fedora/dnf allows WS separated urls (and an optional comma)
           strv::splitRx( line_r, "[,[:blank:]]*[[:blank:]][,[:blank:]]*", [&store_r]( std::string_view w ) {
             if ( ! w.empty() )
-              store_r.push_back( Url(std::string(w)) );
+              store_r.push_back( RawUrl(std::string(w)) );  // RawUrl! to support repo var replacement
           });
         }
 

diff --git a/zypp/repo/RepoVariables.cc b/zypp/repo/RepoVariables.cc
@@ -567,6 +567,16 @@ namespace zypp
         if ( value.schemeIsRawUrl() ) {
           const std::string & replaced { RepoVarExpand()( value.getFragment(), RepoVarsMap::lookup ) };
           newurl = Url( replaced );
+          // Fixup some legacy issue: RepoFileReader encodes a 'proxy=' in .repo in
+          // the Url's Query part. We must restore it from there unless the original Url
+          // defined proxy in it's query part.
+          if ( newurl.getQueryParam( "proxy" ).empty() ) {
+            const std::string & p { value.getQueryParam( "proxy" ) };
+            if ( not p.empty() ) {
+              newurl.setQueryParam( "proxy", p );
+              newurl.setQueryParam( "proxyport", value.getQueryParam( "proxyport" ) );
+            }
+          }
         } else {
           // The traditional
           Url::ViewOptions toReplace = value.getViewOptions() - url::ViewOption::WITH_USERNAME - url::ViewOption::WITH_PASSWORD;