savonrb / akami

Building Web Service Security
MIT License
36 stars 61 forks source link

cert and timestamp can't be together? #15

Open rderoldan1 opened 10 years ago

rderoldan1 commented 10 years ago

@tjarratt I just found something that I consider an issue, if signature and timestamp are true, just signature header is returned

  # Returns the XML for a WSSE header.
    def to_xml
      if signature? and signature.have_document?
         Gyoku.xml wsse_signature1.merge!(hash)
      elsif username_token? && timestamp?
          Gyoku.xml wsse_username_token.merge!(wsu_timestamp) {
          |key, v1, v2| v1.merge!(v2) {
            |key, v1, v2| v1.merge!(v2)
          }
        }
      elsif username_token?
        Gyoku.xml wsse_username_token.merge!(hash)
      elsif timestamp?
        Gyoku.xml wsu_timestamp.merge!(hash)
      else
        ""
      end
    end

The if clause is not considering the case of have signed? and timestamp?, I fix it (in a really ugly way) but for testing purposes it works, I don't know if is not really comment to have timestamps and the cert headers together, but what do you think?

# Returns the XML for a WSSE header.
    def to_xml
      if signature? and signature.have_document?
        if timestamp?
          wsse_signature1 = {}
          wsse_signature1["wsse:Security"] = wsse_signature["wsse:Security"].merge!(wsu_timestamp["wsse:Security"])
          wsse_signature1["wsse:Security"][:order!] << "wsu:Timestamp"
          Gyoku.xml wsse_signature1
        else
          Gyoku.xml wsse_signature.merge!(hash)
        end
      elsif username_token? && timestamp?
          Gyoku.xml wsse_username_token.merge!(wsu_timestamp) {
          |key, v1, v2| v1.merge!(v2) {
            |key, v1, v2| v1.merge!(v2)
          }
        }
      elsif username_token?
        Gyoku.xml wsse_username_token.merge!(hash)
      elsif timestamp?
        Gyoku.xml wsu_timestamp.merge!(hash)
      else
        ""
      end
    end
tjarratt commented 10 years ago

Oh gosh, that if elsif block. The horror!

All joking aside, this is a rather annoying issue that you discovered @rderoldan1. I would have expected this to work, but since support for these various parts of Akami have come about piece by piece I suppose it's natural that this would have been left out.

For now, would you mind issuing this as a pull request along with a spec for this behavior? Once we have some tests, it should be easy to go and refactor this to clean up the implementation.

eiapopeia commented 8 years ago

The patch worked for me. Thanks.

eiapopeia commented 8 years ago

I added a new pull request #25 With this you can have timestamps and signatures at the same time and also have them signed. See the all new specs for usage examples!

jim0020 commented 8 years ago

Is this pull request going to get accepted? I'm having the same issue, monkey patching for now.

brent-clintel commented 7 years ago

Also hoping for this to be accepted.